Miscellaneous NI-RFSG metadata changes #2104
Conversation
Vaishnavigupta1312
requested review from
marcoskirsch and
ni-jfitzger
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #2104 +/- ##
==========================================
+ Coverage 91.34% 91.36% +0.01%
==========================================
Files 66 66
Lines 16292 16318 +26
==========================================
+ Hits 14882 14909 +27
+ Misses 1410 1409 -1
Flags with carried forward coverage won't be shown. Click here to find out more. see 2 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
marcoskirsch
left a comment
marcoskirsch
left a comment
There was a problem hiding this comment.
Confirm the matadata was copied exactly from internal process and not edited at all.
| 'GetAllNamedWaveformNames': { | ||
| 'codegen_method': 'public', | ||
| 'documentation': { | ||
| 'description': '\n Return names of the waveforms present in the memory.\n\n **Supported Devices** :PXIe-5830/5831/5840/5841/5842E\n ' |
There was a problem hiding this comment.
what is up with all this whitespace?
There was a problem hiding this comment.
please review all for similar issues
There was a problem hiding this comment.
Looks like after creating their C Documentation, they copy pasted it for their Python Documentation, at least to some extent.
The issue here is that our plugin for C Documentation removes common indentation, so that you can use it to keep the source readable. But the plugin for Python does not do that.
There was a problem hiding this comment.
Apart from every the line after the first looking a bit indented, I think the generated documentation actually looks okay. It's a minor issue.
https://nirfsg--2104.org.readthedocs.build/en/2104/class.html#methods
I'm guessing codegen trims leading whitespace from first line before adding its own.
|
|
||
| **Default Value:** ArbSampleClockSource.ONBOARD_CLOCK | ||
| **Default Value:** NIRFSG_VAL_ONBOARD_CLOCK_STR |
There was a problem hiding this comment.
Didn't you delete this enum?
There was a problem hiding this comment.
Some of the default value entries got overlooked, will replace it with the actual string value.
| +----------------+--------------------------------------------------------------------------------------------------------------------+ | ||
| | Possible Value | Description | | ||
| +================+====================================================================================================================+ | ||
| | empty | The signal is not exported. | |
There was a problem hiding this comment.
It would probably be better to replace this wih a literal empty string.
And I think you should put quotes around your other possible values.
|
|
||
| Specifies the destination terminal for exporting the Pulse Modulation Event. The Pulse Modulation Event tracks the RF Envelope when Pulse Modulation is Enabled. If this property is set to a value other than `do not export str`, calling NI-RFSG Commit will cause the output terminal to be pulled to the logic level that is the inverse of `exported pulse modulation event active level`. You can tri-state this terminal by setting this property to `do not export str` or by calling `niRFSG Reset`. To set this property, the NI-RFSG device must be in the Configuration state. | ||
|
|
||
| **Default Value:** PulseModulationOutputTerm.PULSE_OUT | ||
| **Default Value:** NIRFSG_VAL_PULSE_OUT_STR |
There was a problem hiding this comment.
Another one that you deleted. Time to audit for the use of these deleted enums in descriptions.
| +----------------+---------------------------------------------------------------------+ | ||
| | Possible Value | Description | | ||
| +================+=====================================================================+ | ||
| | | Pulse modulation video signal is not exported. | |
There was a problem hiding this comment.
What's going on here? Should this value be an empty string?
There was a problem hiding this comment.
This enum value is an empty string, confirmed with the base hapigen metadata for RFSG.
There was a problem hiding this comment.
For strings, empty or otherwise, I really think it's best to use quotes.
| def check_if_waveform_exists(self, waveform_name): | ||
| r'''check_if_waveform_exists | ||
|
|
||
| Returns whether the waveform that you specify as **WAVEFORM_NAME** exists. | ||
| Returns whether the waveform that you specify as WAVEFORM_NAME exists. |
There was a problem hiding this comment.
Another thing to fix in a future PR when you comb over things.
I think what's happening here is that WAVEFORM_NAME isn't the parameter name in C, so it's not getting replaced.
There was a problem hiding this comment.
There is a function parameter in C with the same name but no corresponding constant is there for it as it is not an attribute.
There was a problem hiding this comment.
No, the C parameter name is waveformName.
| 'returns': 'ViStatus', | ||
| 'use_session_lock': False | ||
| }, | ||
| 'SetWaveformBurstStartLocations': { |
There was a problem hiding this comment.
This method is not deleted in the latest internal metadata export. Nor do you seem to have any active internal PRs where this is set to not codegen. Did you hand-edit this file?
There was a problem hiding this comment.
Will raise a PR backporting these changes to AzDO. The file is not hand-edited.
ni-jfitzger
left a comment
ni-jfitzger
left a comment
There was a problem hiding this comment.
Most of the issues that I see are minor and can be handled in a followup PR. However, this metadata does not match our latest internal export nor any internal PR of yours that I can see.
Before this can go in, I need to at least see an approved internal PR with your internal metadata changes. Otherwise, it's very likely that some of your changes will be erased by the next PR with metadata changes.
Additionally, your description did not mention the removal of functions. Please add that to the description along with a short reason for the removal.
And please use an "X" when filling the checkbox in the description. And surround any unchecked items with tildes: ~[ ] Thing that doesn't apply~ so that it renders as crossed out and we know you're not ignoring it.
Is this supposed to say Enum Value names? |
|
Accepting this as is. The documentation issues will be addressed in a followup task. |
ni-jfitzger
left a comment
ni-jfitzger
left a comment
There was a problem hiding this comment.
Documentation issues will still need to be addressed in a followup PR.
4 participants
[ ] I've updated CHANGELOG.md if applicable.[ ] I've added tests applicable for this pull requestWhat does this Pull Request accomplish?
What testing has been done?