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

Optional parameters #704

Merged
merged 23 commits into from
Nov 22, 2024
Merged

Optional parameters #704

merged 23 commits into from
Nov 22, 2024

Conversation

dsavchenko
Copy link
Member

No description provided.

Copy link

codecov bot commented Aug 1, 2024

Codecov Report

Attention: Patch coverage is 74.50980% with 13 lines in your changes missing coverage. Please review.

Project coverage is 62.07%. Comparing base (d1d020b) to head (204f7d2).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
cdci_data_analysis/analysis/parameters.py 76.00% 6 Missing ⚠️
cdci_data_analysis/analysis/queries.py 60.00% 4 Missing ⚠️
cdci_data_analysis/flask_app/dispatcher_query.py 66.66% 2 Missing ⚠️
cdci_data_analysis/analysis/instrument.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #704      +/-   ##
==========================================
+ Coverage   62.06%   62.07%   +0.01%     
==========================================
  Files          50       50              
  Lines        9139     9164      +25     
==========================================
+ Hits         5672     5689      +17     
- Misses       3467     3475       +8     

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


🚨 Try these New Features:

@dsavchenko
Copy link
Member Author

Tests in integral plugin fail on metadata requests, an exception occurs in oda_api https://github.com/oda-hub/oda_api/blob/02e47e3670542e763f381364c4a87de99a2a9de9/oda_api/api.py#L920-L924
This is due to a bit weird structure of the response, which is json list, but all queries are represented there by already json-encoded strings. Now, these strings contain true/false json representation of boolean, which don't eval as python expression.

It may be fixed by adding other replaces (true->True), but I don't really understand why it's implemented like this anyway. Probably just legacy.
@volodymyrss do you have an idea, probably there are additional reasons for this?

@volodymyrss
Copy link
Member

Tests in integral plugin fail on metadata requests, an exception occurs in oda_api https://github.com/oda-hub/oda_api/blob/02e47e3670542e763f381364c4a87de99a2a9de9/oda_api/api.py#L920-L924 This is due to a bit weird structure of the response, which is json list, but all queries are represented there by already json-encoded strings. Now, these strings contain true/false json representation of boolean, which don't eval as python expression.

It may be fixed by adding other replaces (true->True), but I don't really understand why it's implemented like this anyway. Probably just legacy. @volodymyrss do you have an idea, probably there are additional reasons for this?

Looks like legacy code, should be adjusted when possible.

cdci_data_analysis/analysis/queries.py Dismissed Show dismissed Hide dismissed
@dsavchenko dsavchenko marked this pull request as ready for review November 6, 2024 17:05
@volodymyrss volodymyrss linked an issue Nov 8, 2024 that may be closed by this pull request
Copy link
Collaborator

@burnout87 burnout87 left a comment

Choose a reason for hiding this comment

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

Why certain classes within the parameters module are set as optional whereas others not?

cdci_data_analysis/analysis/parameters.py Outdated Show resolved Hide resolved
cdci_data_analysis/analysis/parameters.py Show resolved Hide resolved
Co-authored-by: Gabriele Barni <burnout87@users.noreply.github.com>
@dsavchenko
Copy link
Member Author

dsavchenko commented Nov 15, 2024

Why certain classes within the parameters module are set as optional whereas others not?

There are two parameters declared optional by default: InputProdList and UserCatalog. They are specific to Integral and their value may be set to None.
Also, token may be None, so it's also "optional".
There are also several cases when it's necessary to set optional parameter in tests.

All "normal" parameters are not optional by default.

@burnout87 burnout87 self-requested a review November 18, 2024 09:32
@dsavchenko dsavchenko enabled auto-merge (squash) November 22, 2024 14:56
@dsavchenko dsavchenko merged commit b8868e0 into master Nov 22, 2024
15 checks passed
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.

allow using optional parameters
3 participants