Refactor with API #2
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Amend `README.rst` - Add metadata to `__init__.py` - Remove license/copyright notices from other places as we are now using `README.rst` for this purpose. - Remove `version.py`
pkhalaj
force-pushed
the
feature/refactor-with-api
branch
from
cc6e8aa to
f3eef38
Compare
pkhalaj
force-pushed
the
feature/refactor-with-api
branch
6 times, most recently
from
628ec4a to
fa17e99
Compare
pkhalaj
force-pushed
the
feature/refactor-with-api
branch
from
fa17e99 to
559b20f
Compare
mraspaud
requested changes
May 6, 2024
There was a problem hiding this comment.
Alright, I went through all the files I think :)
First of all, great work with all this! looks clean and organised! Having also documentation autogenerated on readthedocs will help a lot when we work more with this.
I have comments inline, and then some general comments here:
- I'm not too familiar with rest apis and databases, so I was relying on the tests to get a better grip of the provided functionalities here. Do I understand correctly that adding a record to the database is not supported yet?
- For timeouts, we generally use seconds as units in pytroll modules (can be a floating point number)
- Regarding type annotations: I'm not a big fan in general, sorry :) Do you think it would be possible to make them simpler by removing cases we don't need? or refactoring them to simpler types?
- For linting, we use ruff, see the rules we use here for example: https://github.com/pytroll/pytroll-watchers/blob/main/pyproject.toml#L50-L52
Ok, I'll stop here :) but again, great work!
New functionality has been added to the MongoDB class. As a result, the errors are now raised there instead of the API routes. TODO: Documentation and more tests.
mraspaud
requested changes
May 21, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR includes the following: