Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Refactor: rename peer.updated field in torrent detail endpoint #119

Merged
merged 8 commits into from
Nov 29, 2022

Conversation

josecelano
Copy link
Member

@josecelano josecelano commented Nov 25, 2022

TODO:

  • Add an e2e test before refactoring.
  • Refactor the API endpoint to use the resource TorrentResource like AuthKeyResource.
  • Refactor endpoint /api/torrents?offset=:u32&limit=:u32 to use TorrentResource.
  • Add a new field (updated_milliseconds_ago) to the JSON response keeping the old one for backwards compatibility.

@josecelano josecelano linked an issue Nov 25, 2022 that may be closed by this pull request
@josecelano josecelano force-pushed the issue-61-rename-torrent-field-in-endpoint branch from 105dfb2 to bc3d246 Compare November 28, 2022 17:04
@da2ce7
Copy link
Contributor

da2ce7 commented Nov 28, 2022

looks good. however this pull-request will conflict much with #115 , we need to choose what one we merge first.

@josecelano josecelano marked this pull request as ready for review November 28, 2022 19:49
@josecelano
Copy link
Member Author

josecelano commented Nov 28, 2022

looks good. however this pull-request will conflict much with #115 , we need to choose what one we merge first.

hi @da2ce7 I think It's probably better if I rework my changes on top of your PR because:

  • This PR is smaller.
  • Changes are located in very few places and decoupled from the rest.
  • There are more additions than modifications. I think the e2e tests for the API won't be affected too much.
  • I can also learn about Clipply pedantic warnings fixing them in this PR.

This PR is ready for review, but you can wait until I fix conflicts with your changes. The change was very easy, but I've added e2e tests for many endpoints. I think these tests are good because they cover many things, not only the API layer.

Besides, It will be easy to add more API tests.

@josecelano
Copy link
Member Author

josecelano commented Nov 28, 2022

looks good. however this pull-request will conflict much with #115 , we need to choose what one we merge first.

On the other hand 🤔, If we merge first this PR, we are going to have more tests to be sure we have not introduced a regression bug in PR #115.

@da2ce7
Copy link
Contributor

da2ce7 commented Nov 28, 2022

looks good. however this pull-request will conflict much with #115 , we need to choose what one we merge first.

On the other hand thinking, If we merge first this PR, we are going to have more tests to be sure we have not introduced a regression bug in PR #115.

Okay, we merge this first. Then I will try and rebase #115 on top of this. I like having more checks.

Copy link
Contributor

@da2ce7 da2ce7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK e1b84f6

@josecelano
Copy link
Member Author

ACK e1b84f6

@josecelano josecelano merged commit 4d788d2 into develop Nov 29, 2022
@josecelano josecelano deleted the issue-61-rename-torrent-field-in-endpoint branch November 29, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Refactor: rename peer.updated field in torrent detail endpoint
2 participants