-
Notifications
You must be signed in to change notification settings - Fork 93
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
US1198694: add nidcpower.Session.get_channel_names()
#1578
Merged
marcoskirsch
merged 16 commits into
ni:master
from
luisgomes252:us1198694/nidcpower_get_channel_names
May 10, 2021
Merged
Changes from 12 commits
Commits
Show all changes
16 commits
Select commit
Hold shift + click to select a range
0ec9657
Ignore files generated during system test execution
luisgomes252 634382f
Add get_channel_names test
luisgomes252 7edf571
get_channel_names implementation (plus some additional metadata chang…
luisgomes252 a9351c4
Update changelog
luisgomes252 24391c5
Merge branch 'master' into us1198694/nidcpower_get_channel_names
luisgomes252 618d7ea
Merge remote-tracking branch 'origin/master' into us1198694/nidcpower…
luisgomes252 ca843ca
Address code review feedback (https://github.com/ni/nimi-python/pull/…
luisgomes252 c42d8f3
Fix whitespace
luisgomes252 ae9d138
Some additional code review feedback from Marcos
luisgomes252 ed39b57
wait for a different PR to update .gitignore
luisgomes252 db77938
Restore blank line
luisgomes252 9d35348
Take into account python_name parameter overrides when determining re…
luisgomes252 91531fa
Address some additional feedback from Shreyas: 1. always create `pyth…
luisgomes252 e74fc03
Add test for python_name override
luisgomes252 a5643de
Add tests for snake_case / camel_case conversions
luisgomes252 7f65687
Revert changes to _add_is_repeated_capability() based on code review …
luisgomes252 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This whole method is one big hack. We're inferring if a parameter is a repeated capability based on the name of the parameter; unless explicitly denoted. There are historical reasons for it, but I strongly question it nowadays:
Shouldn't we fix this in metadata? Then (I think) we can remove the inferring code and we can remove all this camel case conversion business.
Here's the thing: converting snakecase_to_camelcase in order to reverse the python name is a bit sketchy. It assumes that python name to C name are always camelCase to snake_case. Which happens to be true right now but doesn't really have to be true.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that the right fix, but it involves retrofitting metadata for all the drivers.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Between the comments in this PR, and now the ones here in this PR, this relatively small user story is beginning to feel like a giant exercise of pulling at threads to see how can much can be unraveled. I won't dispute that there was some technical debt in the code that should probably be addressed at some point -- although I'm not the expert here, I can certainly agree that making assumptions on repeated capabilities based on the name of the parameter doesn't sound great. But I really wonder whether an "inner source" project in which we were asked to help complete the CXP API changes for nidcpower is the best vehicle for solving all this tech debt. After all, we also have a lot of tech debt in TSM that is waiting for attention too. The user story was ostensibly about adding support for instrument-based repeated capabilities for attributes (not even for function parameters). Here we are, almost a month later, and I'm no longer sure what the scope of the user story is any more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I stand corrected. This PR isn't even for the repeated capabilities user story (it's been so long, all these PRs are beginning to blur into each other). This one was just for the
get_channel_names
entry point, which isn't even related to repeated capabilities. It just exposed this issue because I needed to add support in the metadata forpython_name
in function parameters, which then introduced the need to update the_repeated_capability_parameter_names
looksups . So I guess it's more relevant to Marcos' comment than my first comment implied. But we're still pretty far afield from the original user story.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spoke with @luisgomes252 offline and suggested to remove the workaround in
_add_is_repeated_capability
and addis_repeated_capability = false
to nidcpower metadata to prevent the rep cap tip from getting added to the docstring.The inferring logic will still be present and we should clean it up, not that's outside the scope of this PR.
Even setting
is_repeated_capability = false
can be done in a separate PR since others are blocked on this PR going in.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on the conversation with @sbethur, I've reverted the changes that I had made to use the python_name metadata in determining whether to add the repeated capability. Currently, get_channel_names is incorrectly being generated with metadata, but I'll fix that by updating the dcpower metadata in a subsequent PR so that I can get this one submitted as soon as possible, because a) other PRs are being gated by this one, and b) if I have to grab a new metadata export, that will also pull in a bunch of unrelated changes into this diff. So I'd like to split the PRs, if possible. @marcoskirsch: if you agree, and you have no other objections, please approve this PR when you have a chance.