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

API: Move command module into parametes and undocument #4286

Merged
merged 4 commits into from
Jun 20, 2022

Conversation

jenshnielsen
Copy link
Collaborator

To make this possible also move Function into Parameters where it more naturally belongs

@codecov
Copy link

codecov bot commented Jun 20, 2022

Codecov Report

Merging #4286 (a9612ed) into master (3acdc31) will decrease coverage by 0.01%.
The diff coverage is 91.37%.

@@            Coverage Diff             @@
##           master    #4286      +/-   ##
==========================================
- Coverage   68.40%   68.39%   -0.02%     
==========================================
  Files         249      251       +2     
  Lines       30941    30943       +2     
==========================================
- Hits        21166    21164       -2     
- Misses       9775     9779       +4     

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.

move Function into Parameters where it more naturally belongs

could you share the reasoning behind this? i'm happy to share mine: it seems to me that Function belongs to the Instruments because it's an instrument-specific feature, and does not relate to parameters at all. Parameters don't use it and Functions dont use parameters, however Instruments use Functions and Function is means to live on an instrument (even as per it's docstring). Command on the other hand is indeed a thing that only Parameters happen to use, so Command belonging to Parameters makes a lot of sense (and it's not a package-scope utility, which is the original motivation of the PR, right?).

@jenshnielsen
Copy link
Collaborator Author

could you share the reasoning behind this? i'm happy to share mine: it seems to me that Function belongs to the Instruments because it's an instrument-specific feature, and does not relate to parameters at all. Parameters don't use it and Functions dont use parameters, however Instruments use Functions and Function is means to live on an instrument (even as per it's docstring). Command on the other hand is indeed a thing that only Parameters happen to use, so Command belonging to Parameters makes a lot of sense (and it's not a package-scope utility, which is the original motivation of the PR, right?).

Both parameters and functions are things that the instrument uses at exactly the same level. 1Both make use of Command so it makes sense to group all of these things together. In reality functions is a limited version of a parameter. I agree that the nameing of the module paraemters is not optimal from this way but since Functions is a minor thing that we really want to get rid of and deprecate (or at least strongly discourage the use of) I am not to worried

@astafan8
Copy link
Contributor

Both make use of Command so it makes sense to group all of these things together

arggh, missed that Function uses Command. ok, no things make sense, thanks!

@jenshnielsen
Copy link
Collaborator Author

Also see #4292 which should fully eliminate the use of Functions

@jenshnielsen
Copy link
Collaborator Author

bors merge

@bors bors bot merged commit 4b47875 into microsoft:master Jun 20, 2022
@jenshnielsen jenshnielsen deleted the api/move_command branch June 20, 2022 13:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants