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.

Sign up for GitHub

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

Jump to bottom

add function to get guids by run spec #3863

Merged
merged 6 commits into from
Feb 10, 2022
Merged

add function to get guids by run spec #3863

merged 6 commits into from
Feb 10, 2022

Conversation

jenshnielsen
Copy link
Collaborator

@jenshnielsen jenshnielsen commented Jan 25, 2022 •
edited

And deprecate and remove use of get_guids_from_run_spec.

  • Add to public api
  • Tests

@jenshnielsen jenshnielsen force-pushed the get_guid_by_run_spec_public branch 3 times, most recently from d862a42 to b97fda9 Compare January 25, 2022 09:42
@codecov
Copy link

codecov bot commented Jan 25, 2022 •
edited

Codecov Report

Merging #3863 (40a320b) into master (02c052e) will increase coverage by 0.00%.
The diff coverage is 95.45%.

@@           Coverage Diff           @@
##           master    #3863   +/-   ##
=======================================
  Coverage   65.77%   65.78%           
=======================================
  Files         228      228           
  Lines       31103    31113   +10     
=======================================
+ Hits        20459    20468    +9     
- Misses      10644    10645    +1

@Dominik-Vogel
Copy link
Contributor

@jenshnielsen this looks good to me. The only thing that I think is worth considering is what we do about the get_guids_from_runspec. It seems quite confusing to have two functions with such similar names and such similar actions. I am aware of the fact that only one of them is public api, but also for devs this is very confusing.
We could have the code for querying and filtering in withing the more general by_runspec and let from_runspec call that one, as well as deprecating from_runspec with the reference to use by_runspec. What do you think?

@jenshnielsen
Copy link
Collaborator Author

So we want to leave the code for querying in that module but we could rename the get_guids_from_run_spec in that module to something else _query_guids_from_run_specs perhaps and leave get_guids_from_run_spec around in the queries module (but deprecated)

@astafan8 astafan8 added this to the 0.33.0 milestone Jan 26, 2022
@jenshnielsen
Copy link
Collaborator Author

@astafan8 Can you see if this api makes sense. If so I will finish this up with some tests

astafan8
astafan8 reviewed Jan 28, 2022
View reviewed changes
Copy link
Contributor

@astafan8 astafan8 left a comment

Choose a reason for hiding this comment

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

yes, the API looks good!

@jenshnielsen jenshnielsen marked this pull request as ready for review February 7, 2022 12:54
Dominik-Vogel
Dominik-Vogel approved these changes Feb 7, 2022
View reviewed changes
Copy link
Contributor

@Dominik-Vogel Dominik-Vogel left a comment

Choose a reason for hiding this comment

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

@jenshnielsen so we are basically renaming the function because we have connection as a non-keyword argument, right? I think it does make a lot of sense to me, how you implemented this change.

@jenshnielsen
Copy link
Collaborator Author

@Dominik-Vogel yes and also to match load_by_id, load_by_run_spec etc

@jenshnielsen
Copy link
Collaborator Author

bors merge

@bors
Copy link
Contributor

bors bot commented Feb 10, 2022

Build succeeded:

@bors bors bot merged commit fb16547 into microsoft:master Feb 10, 2022
@jenshnielsen jenshnielsen deleted the get_guid_by_run_spec_public branch February 14, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@astafan8 astafan8 astafan8 left review comments

@Dominik-Vogel Dominik-Vogel Dominik-Vogel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
0.33.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@jenshnielsen @Dominik-Vogel @astafan8