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

Enhance code quality #6

Merged
merged 21 commits into from
May 31, 2024

Conversation

pkhalaj
Copy link
Collaborator

@pkhalaj pkhalaj commented May 24, 2024

This PR is about

  • improving the quality of docstrings and modifying to conf.py to exclude unused special members.
  • improving type checks and validation.
  • minor code refactoring to improve readability
  • adding some tests and associated testing utilities (TODO: more will be added in a forthcoming PR).

Copy link

codecov bot commented May 24, 2024

Codecov Report

Attention: Patch coverage is 95.59471% with 10 lines in your changes missing coverage. Please review.

Project coverage is 89.96%. Comparing base (b24bc53) to head (781891d).
Report is 64 commits behind head on master.

Files with missing lines Patch % Lines
trolldb/api/api.py 57.14% 6 Missing ⚠️
trolldb/cli.py 89.28% 3 Missing ⚠️
trolldb/test_utils/mongodb_database.py 98.63% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master       #6      +/-   ##
==========================================
+ Coverage   89.07%   89.96%   +0.89%     
==========================================
  Files          24       24              
  Lines         842      947     +105     
==========================================
+ Hits          750      852     +102     
- Misses         92       95       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@pkhalaj pkhalaj force-pushed the enhancement/more-tests-and-cleanup branch 2 times, most recently from 4982384 to ec787e7 Compare May 24, 2024 16:54
- Fix typos.
- Fix broken links.
- Improve readability.
Update conf.py with functionalities to specify which special members have to be kept in the documentation.
The name of the `pipelines` module was mistakenly spelled as `piplines`.
There were several places that we had used `mock data` to refer to data generated for testing purposes. This was misleading as we are actually generating the data and the API calls are not mocked. Example `mock_database` is renamed to `test_database`.
As a result, `test_utils` and the `queries` route handler have been also updated.
@pkhalaj pkhalaj force-pushed the enhancement/more-tests-and-cleanup branch 3 times, most recently from 03506ae to 4f426ad Compare May 29, 2024 14:43
@pkhalaj pkhalaj force-pushed the enhancement/more-tests-and-cleanup branch from 4f426ad to 76fe40e Compare May 29, 2024 14:58
The new version of FastAPI recommends using `Annotated` for queries. This also resolves the issue with ruff complaining about `Query(None)`.
@pkhalaj pkhalaj force-pushed the enhancement/more-tests-and-cleanup branch from 2c8a672 to a4298f5 Compare May 29, 2024 17:14
This led to the removal of some redundant type checks.
The bug had to do with multiple deletion in the recorder. The log message was added to the auto exception handler.
Add two auxiliary methods which simplify the implementation of the main method.
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

Just some minor comments, otherwise looks good!

trolldb/cli.py Outdated Show resolved Hide resolved
trolldb/config/config.py Outdated Show resolved Hide resolved
trolldb/database/mongodb.py Outdated Show resolved Hide resolved
trolldb/tests/tests_api/test_api.py Outdated Show resolved Hide resolved
The function is named `make_query_string`
The old name of the function was `parse_config_yaml_file`. The function now accepts a number of different types for the input arg `file`.
This logs and catches unhandled exceptions.
trolldb/config/config.py Outdated Show resolved Hide resolved
trolldb/config/config.py Outdated Show resolved Hide resolved
trolldb/config/config.py Outdated Show resolved Hide resolved
trolldb/api/api.py Outdated Show resolved Hide resolved
trolldb/api/api.py Outdated Show resolved Hide resolved
- Remove type hint for `file`.
- Amend docstring.
- Remove explicit exception handling for parsing and reading the file.
As a result, the signature of `run_server()` and
`api_server_process_context()` have been changed.
They now only accept a `config` object of type
`AppConfig`.
@pkhalaj pkhalaj force-pushed the enhancement/more-tests-and-cleanup branch from bf6a73d to 506f122 Compare May 31, 2024 09:31
The dependencies are now brought after the first main dependent function that uses them.
@pkhalaj pkhalaj requested a review from mraspaud May 31, 2024 09:52
Copy link
Member

@mraspaud mraspaud left a comment

Choose a reason for hiding this comment

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

LGTM

@mraspaud mraspaud merged commit 1f0b8d0 into pytroll:master May 31, 2024
6 checks passed
@pkhalaj pkhalaj deleted the enhancement/more-tests-and-cleanup branch May 31, 2024 10:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants