US1198694: add `nidcpower.Session.get_channel_names()` #1578

luisgomes252 · 2021-04-07T22:51:06Z

This contribution adheres to CONTRIBUTING.md.
I've updated CHANGELOG.md if applicable.
I've added tests applicable for this pull request

What does this Pull Request accomplish?

Add get_channel_names() for NI-DCPower API

List issues fixed by this Pull Request below, if any.

Closes #1588

What testing has been done?

Added new system test and ran all unit tests. System tests will be run by CI.

…es that hadn't yet been codegen'd)

nimi-bot · 2021-04-07T22:52:03Z

Can one of the admins verify this patch?

marcoskirsch · 2021-04-09T13:46:09Z

FYI merge conflict @luisgomes252

generated/nidcpower/nidcpower/_library.py

sbethur · 2021-04-09T18:21:07Z

ok to test

codecov · 2021-04-09T18:23:30Z

Codecov Report

Merging #1578 (a5643de) into master (661c2f3) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1578      +/-   ##
==========================================
- Coverage   91.68%   91.67%   -0.01%     
==========================================
  Files          20       20              
  Lines        3463     3509      +46     
==========================================
+ Hits         3175     3217      +42     
- Misses        288      292       +4

Flag	Coverage Δ
codegenunittests	`88.34% <100.00%> (+0.06%)`	⬆️
nifakeunittests	`96.36% <ø> (ø)`
nimodinstunittests	`95.37% <ø> (ø)`
nitclkunittests	`95.61% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
build/helper/__init__.py	`100.00% <100.00%> (ø)`
build/helper/helper.py	`91.33% <100.00%> (+3.04%)`	⬆️
build/helper/metadata_add_all.py	`80.68% <100.00%> (-0.67%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 661c2f3...a5643de. Read the comment docs.

marcoskirsch · 2021-04-13T22:50:44Z

ok to test

src/nidcpower/metadata/attributes.py

src/nidcpower/metadata/functions.py

src/nidcpower/system_tests/test_system_nidcpower.py

CHANGELOG.md

src/nidcpower/metadata/functions.py

src/nidcpower/system_tests/test_system_nidcpower.py

src/nidcpower/metadata/functions.py

generated/nidcpower/nidcpower/session.py

CHANGELOG.md

sbethur · 2021-04-14T18:02:14Z

test this please

sbethur · 2021-04-19T19:33:15Z

@luisgomes252, a system test is failing:

multi_instrument_session = nidcpower.Session(resource_name='PXI1Slot2,PXI1Slot5', channels='', reset=False, options='Simulate=1, DriverSetup=Model:4162; BoardType:PXIe')

    def test_get_channel_names(multi_instrument_session):
        # Once we have support for independent channels, we should update this test to include
        # the instrument names in the expected channel names -- or possibly add a separate test
        # expected_string = ['{0}/{1}'.format(instruments[0], x) for x in range(12)]
        expected_string = ['{0}'.format(x) for x in range(12)]
        channel_indices = ['0-1, 2, 3:4', 5, (6, 7), range(8, 10), slice(10, 12)]
>       assert multi_instrument_session.get_channel_names(indices=channel_indices) == expected_string

..\..\src\nidcpower\system_tests\test_system_nidcpower.py:48: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _

…_get_channel_names

.gitignore

docs/nidcpower/class.rst

src/nidcpower/system_tests/test_system_nidcpower.py

sbethur · 2021-04-26T15:44:03Z

Please update the description and referenced issue.

Also, you can use a particular format to auto-close the issue when PR is merged: https://docs.github.com/en/github/managing-your-work-on-github/linking-a-pull-request-to-an-issue

sbethur · 2021-04-27T16:33:18Z

Linter is complaining:

src/nidcpower/system_tests/test_system_nidcpower.py:7:1: E302 expected 2 blank lines, found 1

@pytest.fixture(scope='function')

^

ERROR: InvocationError for command /home/travis/build/ni/nimi-python/.tox/64/py38-flake8/bin/flake8 --config=./tox.ini src/nidcpower/system_tests/ src/nidcpower/examples/ (exited with code 1)

luisgomes252 · 2021-04-27T17:17:43Z

Linter is complaining:

src/nidcpower/system_tests/test_system_nidcpower.py:7:1: E302 expected 2 blank lines, found 1

@pytest.fixture(scope='function')

^

ERROR: InvocationError for command /home/travis/build/ni/nimi-python/.tox/64/py38-flake8/bin/flake8 --config=./tox.ini src/nidcpower/system_tests/ src/nidcpower/examples/ (exited with code 1)

Argh. I keep doing this.

Where did you go to see the Linter output?

sbethur · 2021-04-27T18:19:33Z

Linter is complaining:

src/nidcpower/system_tests/test_system_nidcpower.py:7:1: E302 expected 2 blank lines, found 1

@pytest.fixture(scope='function')

^

ERROR: InvocationError for command /home/travis/build/ni/nimi-python/.tox/64/py38-flake8/bin/flake8 --config=./tox.ini src/nidcpower/system_tests/ src/nidcpower/examples/ (exited with code 1)

Argh. I keep doing this.

Where did you go to see the Linter output?

Here's the link to it: https://travis-ci.org/github/ni/nimi-python/builds/768570772

…peated capabilities

build/helper/metadata_add_all.py

…on_name_override` and 2. avoid mixing camel-cased and snake-cased parameter names when comparing them

build/helper/metadata_add_all.py

sbethur · 2021-05-03T17:09:55Z

test this please

marcoskirsch · 2021-05-03T18:05:20Z

build/helper/metadata_add_all.py

@@ -271,7 +275,11 @@ def _add_render_in_session_base(f):
 def _add_is_repeated_capability(parameter):


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.

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.

Agreed, that the right fix, but it involves retrofitting metadata for all the drivers.

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.

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 for python_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.

Spoke with @luisgomes252 offline and suggested to remove the workaround in _add_is_repeated_capability and add is_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.

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.

sbethur · 2021-05-03T18:48:25Z

test this please

…feedback. I will fix the incorrect repeated capability for the parameter names later, by adding metadata is_repeated_capability tags to the get_channel_names parameters.

sbethur · 2021-05-06T16:32:56Z

test this please

luisgomes252 added 4 commits April 5, 2021 14:13

Ignore files generated during system test execution

0ec9657

Add get_channel_names test

634382f

get_channel_names implementation (plus some additional metadata chang…

7edf571

…es that hadn't yet been codegen'd)

Update changelog

a9351c4

luisgomes252 requested review from marcoskirsch and sbethur as code owners April 7, 2021 22:51

marcoskirsch changed the title ~~Us1198694/nidcpower get channel names~~ US1198694 add nidcpower.Session.get_channel_names() Apr 9, 2021

marcoskirsch requested changes Apr 9, 2021

View reviewed changes

generated/nidcpower/nidcpower/_library.py Outdated Show resolved Hide resolved

Merge branch 'master' into us1198694/nidcpower_get_channel_names

24391c5

luisgomes252 requested a review from marcoskirsch April 13, 2021 21:17

marcoskirsch requested changes Apr 13, 2021

View reviewed changes

src/nidcpower/metadata/attributes.py Outdated Show resolved Hide resolved

src/nidcpower/metadata/functions.py Outdated Show resolved Hide resolved

src/nidcpower/system_tests/test_system_nidcpower.py Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

sbethur suggested changes Apr 14, 2021

View reviewed changes

luisgomes252 added 2 commits April 21, 2021 16:59

Merge remote-tracking branch 'origin/master' into us1198694/nidcpower…

618d7ea

…_get_channel_names

Address code review feedback (ni#1578)

ca843ca

luisgomes252 requested review from sbethur and marcoskirsch April 22, 2021 15:57

Fix whitespace

c42d8f3

marcoskirsch requested changes Apr 23, 2021

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

docs/nidcpower/class.rst Show resolved Hide resolved

src/nidcpower/system_tests/test_system_nidcpower.py Outdated Show resolved Hide resolved

luisgomes252 added 2 commits April 26, 2021 16:57

Some additional code review feedback from Marcos

ae9d138

wait for a different PR to update .gitignore

ed39b57

luisgomes252 requested a review from marcoskirsch April 27, 2021 14:55

luisgomes252 mentioned this pull request Apr 27, 2021

Update the index list documentation to be python-specific #1595

Open

sbethur approved these changes Apr 27, 2021

View reviewed changes

Restore blank line

db77938

Take into account python_name parameter overrides when determining re…

9d35348

…peated capabilities

sbethur reviewed Apr 28, 2021

View reviewed changes

build/helper/metadata_add_all.py Outdated Show resolved Hide resolved

luisgomes252 added 3 commits April 28, 2021 18:51

Address some additional feedback from Shreyas: 1. always create `pyth…

91531fa

…on_name_override` and 2. avoid mixing camel-cased and snake-cased parameter names when comparing them

Add test for python_name override

e74fc03

Add tests for snake_case / camel_case conversions

a5643de

sbethur reviewed May 3, 2021

View reviewed changes

build/helper/metadata_add_all.py Outdated Show resolved Hide resolved

marcoskirsch requested changes May 3, 2021

View reviewed changes

Revert changes to _add_is_repeated_capability() based on code review …

7f65687

…feedback. I will fix the incorrect repeated capability for the parameter names later, by adding metadata is_repeated_capability tags to the get_channel_names parameters.

luisgomes252 requested a review from marcoskirsch May 6, 2021 15:58

luisgomes252 changed the title ~~US1198694 add nidcpower.Session.get_channel_names()~~ US1198694: add nidcpower.Session.get_channel_names() May 7, 2021

marcoskirsch approved these changes May 10, 2021

View reviewed changes

marcoskirsch merged commit 753c21f into ni:master May 10, 2021

luisgomes252 mentioned this pull request May 10, 2021

Use explicit metadata attribute to avoid adding repeated capability #1601

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

US1198694: add `nidcpower.Session.get_channel_names()` #1578

US1198694: add `nidcpower.Session.get_channel_names()` #1578

luisgomes252 commented Apr 7, 2021 •

edited

Loading

nimi-bot commented Apr 7, 2021

marcoskirsch commented Apr 9, 2021

sbethur commented Apr 9, 2021

codecov bot commented Apr 9, 2021 •

edited

Loading

marcoskirsch commented Apr 13, 2021

sbethur commented Apr 14, 2021

sbethur commented Apr 19, 2021

sbethur commented Apr 26, 2021

sbethur commented Apr 27, 2021

luisgomes252 commented Apr 27, 2021

sbethur commented Apr 27, 2021

sbethur commented May 3, 2021

marcoskirsch May 3, 2021

sbethur May 3, 2021

luisgomes252 May 3, 2021

luisgomes252 May 3, 2021

sbethur May 5, 2021

luisgomes252 May 6, 2021

sbethur commented May 3, 2021

sbethur commented May 6, 2021

		@@ -271,7 +275,11 @@ def _add_render_in_session_base(f):
		def _add_is_repeated_capability(parameter):

US1198694: add nidcpower.Session.get_channel_names() #1578

US1198694: add nidcpower.Session.get_channel_names() #1578

Conversation

luisgomes252 commented Apr 7, 2021 • edited Loading

What does this Pull Request accomplish?

List issues fixed by this Pull Request below, if any.

What testing has been done?

nimi-bot commented Apr 7, 2021

marcoskirsch commented Apr 9, 2021

sbethur commented Apr 9, 2021

codecov bot commented Apr 9, 2021 • edited Loading

Codecov Report

marcoskirsch commented Apr 13, 2021

sbethur commented Apr 14, 2021

sbethur commented Apr 19, 2021

sbethur commented Apr 26, 2021

sbethur commented Apr 27, 2021

luisgomes252 commented Apr 27, 2021

sbethur commented Apr 27, 2021

sbethur commented May 3, 2021

marcoskirsch May 3, 2021

Choose a reason for hiding this comment

sbethur May 3, 2021

Choose a reason for hiding this comment

luisgomes252 May 3, 2021

Choose a reason for hiding this comment

luisgomes252 May 3, 2021

Choose a reason for hiding this comment

sbethur May 5, 2021

Choose a reason for hiding this comment

luisgomes252 May 6, 2021

Choose a reason for hiding this comment

sbethur commented May 3, 2021

sbethur commented May 6, 2021

US1198694: add `nidcpower.Session.get_channel_names()` #1578

US1198694: add `nidcpower.Session.get_channel_names()` #1578

luisgomes252 commented Apr 7, 2021 •

edited

Loading

codecov bot commented Apr 9, 2021 •

edited

Loading