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

prod_name in get_par_by_name #537

Merged
merged 8 commits into from
May 21, 2023
Merged

prod_name in get_par_by_name #537

merged 8 commits into from
May 21, 2023

Conversation

dsavchenko
Copy link
Member

No description provided.

@volodymyrss
Copy link
Member

it seems to make sense, but could you please explain a little bit the motivation?

@dsavchenko
Copy link
Member Author

it seems to make sense, but could you please explain a little bit the motivation?

I came up with this in the context of oda-hub/dispatcher-plugin-nb2workflow#47. When I build a request to the backend, I want to be sure that the parameter I'm working with is coming from the proper product. If parameters with the same name are annotated differently in the notebooks, they are not equivalent in the dispatcher. It addresses the TODO comment

# TODO: this picks the last one if there are many?..
for this case.

It's not a strictly necessary change, I guess, as the problem in oda-hub/dispatcher-plugin-nb2workflow#47 concerns SourceQuery parameters, but it's cleaner to use it this way

@codecov
Copy link

codecov bot commented May 12, 2023

Codecov Report

Merging #537 (5a26718) into master (fe7aaf5) will increase coverage by 0.04%.
The diff coverage is 76.66%.

@@            Coverage Diff             @@
##           master     #537      +/-   ##
==========================================
+ Coverage   62.27%   62.32%   +0.04%     
==========================================
  Files          45       45              
  Lines        7606     7620      +14     
==========================================
+ Hits         4737     4749      +12     
- Misses       2869     2871       +2     
Impacted Files Coverage Δ
cdci_data_analysis/analysis/instrument.py 81.41% <68.42%> (-0.07%) ⬇️
cdci_data_analysis/analysis/queries.py 61.35% <90.90%> (+0.54%) ⬆️

@volodymyrss volodymyrss self-requested a review May 12, 2023 10:06
volodymyrss
volodymyrss previously approved these changes May 12, 2023
@volodymyrss
Copy link
Member

something breaks though

@volodymyrss
Copy link
Member

oh, do we already have test for this? could you add one?

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.

Very useful thanks.

EDIT: also a dedicated test would be really useful, something was already done here:

def test_repeating_parameters(add_duplicate):

cdci_data_analysis/analysis/instrument.py Outdated Show resolved Hide resolved
cdci_data_analysis/analysis/instrument.py Outdated Show resolved Hide resolved
cdci_data_analysis/analysis/instrument.py Outdated Show resolved Hide resolved
dsavchenko and others added 5 commits May 12, 2023 13:11
Co-authored-by: Gabriele Barni <burnout87@users.noreply.github.com>
Co-authored-by: Gabriele Barni <burnout87@users.noreply.github.com>
parameters_list = [p1, p2]
with pytest.raises(RuntimeError):
product_query = ProductQuery("test_product_query", parameters_list=parameters_list)

Check notice

Code scanning / CodeQL

Unused local variable Note

Variable product_query is not used.
@dsavchenko
Copy link
Member Author

could you merge please?

@volodymyrss
Copy link
Member

I add you to maintainers, I think you add enough code. I merge though

@volodymyrss volodymyrss merged commit 74e260a into master May 21, 2023
16 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.

None yet

3 participants