Fix several API typos and document the process in CONTRIBUTING.md #943
There are no files selected for viewing
This file contains hidden or 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 hidden or 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
|
Collaborator
There was a problem hiding this comment. Please don't document how to fix specific types of typos. They should be rare and documenting this makes the document harder to read and maintain. I think it's ok to include some basic guidelines:
|
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,81 @@ $ poetry install --with docs | |
| $ poetry run sphinx-build -b html docs docs\_build | ||
| ``` | ||
|
|
||
| # Fixing API Typos | ||
|
|
||
| Public API names may contain typos. If you identify a typo in such names, the remediation path | ||
| depends on the type of API name. Subsections below cover some API name categories. If you | ||
| encounter a typo that does not fit any documented category, follow the general spirit of the | ||
| existing subsections and document the new approach in a new subsection below. | ||
|
|
||
| ## Typos in Python Property Names | ||
|
|
||
| Python property names are defined in | ||
| [`src/codegen/utilities/attribute_helpers.py`](src/codegen/utilities/attribute_helpers.py) and | ||
| generated from metadata. Because renaming a public property is a breaking change, typos are fixed | ||
| using a deprecation process rather than being corrected immediately. | ||
|
|
||
| To fix a typo in a generated property: | ||
|
|
||
| 1. Add an entry to `DEPRECATED_ATTRIBUTES` in | ||
| [`src/codegen/utilities/attribute_helpers.py`](src/codegen/utilities/attribute_helpers.py): | ||
| ```python | ||
| "old_typo_name": {"new_name": "correct_name", "deprecated_in": "<current version>"}, | ||
| ``` | ||
| The code generator will automatically emit a shim property for the old name that delegates to | ||
| the new name and raises `DeprecationWarning` on access. | ||
|
|
||
| 2. Regenerate the code: | ||
| ```sh | ||
| $ poetry run python src/codegen --dest generated/nidaqmx | ||
| ``` | ||
|
|
||
| 3. Update the cspell dictionary in | ||
| [`.config/cspell/daqmx-api-elements.txt`](.config/cspell/daqmx-api-elements.txt): | ||
| - If the typo word is already present, update its comment to | ||
| `# Deprecated; see attribute_helpers.py`. Otherwise, add it with that comment. | ||
| - The entry must remain in the dictionary for as long as the deprecated shim property exists in | ||
| the generated code. | ||
| - Add the correct name if it is not already known to cspell. | ||
|
|
||
| 4. Add a bullet point to the `CHANGELOG.md` section for the current version describing the rename | ||
| and deprecation. | ||
|
|
||
| For typos in handwritten functions or constants, apply the `@deprecation.deprecated` decorator | ||
| directly and add a correctly named alias. See | ||
| [`src/handwritten/errors.py`](src/handwritten/errors.py) for examples. | ||
|
|
||
| ## Typos in Error Code Names | ||
|
|
||
| Error code names are derived from the NI-DAQmx C API metadata in | ||
| [`src/codegen/metadata/enums.py`](src/codegen/metadata/enums.py) and historically were used | ||
| verbatim as the Python names. This means typos in the C API shipped as-is in the Python API, and | ||
| renaming them requires preserving the old names as aliases for backward compatibility. | ||
|
|
||
| To fix a typo in an error code name: | ||
|
|
||
| 1. Add an entry to `ERROR_CODE_NAME_SUBSTITUTIONS` in | ||
| [`src/codegen/utilities/enum_helpers.py`](src/codegen/utilities/enum_helpers.py): | ||
| ```python | ||
| "C_API_NAME_WITH_TYPO": "CORRECTED_NAME", | ||
| ``` | ||
| The code generator will emit the corrected name as the primary member and keep the old typo | ||
| name as an alias pointing to it, annotated with a comment noting it is kept for backward | ||
| compatibility. | ||
|
|
||
| 2. Regenerate the code: | ||
| ```sh | ||
| $ poetry run python src/codegen --dest generated/nidaqmx | ||
| ``` | ||
|
|
||
| 3. Update the cspell dictionary in | ||
| [`.config/cspell/daqmx-api-elements.txt`](.config/cspell/daqmx-api-elements.txt): | ||
| - If the typo word is already present, update its comment to | ||
| `# Typo in C API; remapped via enum_helpers.py`. Otherwise, add it with that comment. | ||
|
|
||
| 4. Add a bullet point to `CHANGELOG.md` for the current version noting the rename and that the | ||
| old name is preserved as an alias for backward compatibility. | ||
|
|
||
| # Branching Policy | ||
|
|
||
| Active development for the next release occurs on the `master` branch. | ||
|
|
@@ -152,7 +227,7 @@ can be verified once that has been completed. | |
| - If the new version number is incorrect, update it by posting and committing a suggestion. | ||
| 8. Create a PR adding a section to `CHANGELOG.md` for the new version with empty subsections. | ||
|
|
||
| # Updating gRPC Stubs When the .proto File Is Modified | ||
|
|
||
| The `generated\nidaqmx\_stubs` directory contains the auto-generated Python files based on the NI-DAQmx protobuf (`.proto`) file. | ||
|
|
||
|
|
||
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -170,7 +170,9 @@ class DAQmxErrors(IntEnum): | |
| OPERATION_ABORTED = -201452 | ||
| TWO_PORTS_REQUIRED = -201451 | ||
| DEVICE_DOES_NOT_SUPPORT_CDAQ_SYNC_CONNECTIONS = -201450 | ||
| INVALID_CDAQ_SYNC_PORT_CONNECTION_FORMAT = -201449 | ||
| # Typo in C API; kept for backward compatibility | ||
| INVALIDC_DAQ_SYNC_PORT_CONNECTION_FORMAT = INVALID_CDAQ_SYNC_PORT_CONNECTION_FORMAT | ||
| ROSETTE_MEASUREMENTS_NOT_SPECIFIED = -201448 | ||
| INVALID_NUM_OF_PHYS_CHANS_FOR_DELTA_ROSETTE = -201447 | ||
| INVALID_NUM_OF_PHYS_CHANS_FOR_TEE_ROSETTE = -201446 | ||
|
|
@@ -187,7 +189,9 @@ class DAQmxErrors(IntEnum): | |
| CANT_PRIME_WITH_EMPTY_BUFFER = -201435 | ||
| CONFIG_FAILED_BECAUSE_WATCHDOG_EXPIRED = -201434 | ||
| WRITE_FAILED_BECAUSE_WATCHDOG_CHANGED_LINE_DIRECTION = -201433 | ||
| MULTIPLE_SUBSYSTEM_CALIBRATION = -201432 | ||
| # Typo in C API; kept for backward compatibility | ||
| MULTIPLE_SUBSYTEM_CALIBRATION = MULTIPLE_SUBSYSTEM_CALIBRATION | ||
| INCORRECT_CHANNEL_FOR_OFFSET_ADJUSTMENT = -201431 | ||
| INVALID_NUM_REF_VOLTAGES_TO_WRITE = -201430 | ||
| START_TRIG_DELAY_WITH_DSA_MODULE = -201429 | ||
|
|
@@ -223,7 +227,9 @@ class DAQmxErrors(IntEnum): | |
| MODULE_MISMATCH_IN_SAME_TIMED_TASK = -201399 | ||
| INVALID_ATTRIBUTE_VALUE_POSSIBLY_DUE_TO_OTHER_ATTRIBUTE_VALUES = -201398 | ||
| CHANGE_DETECTION_STOPPED_TO_PREVENT_DEVICE_HANG = -201397 | ||
| FILTER_DELAY_REMOVAL_NOT_POSSIBLE_WITH_ANALOG_TRIGGER = -201396 | ||
| # Typo in C API; kept for backward compatibility | ||
| FILTER_DELAY_REMOVAL_NOT_POSSSIBLE_WITH_ANALOG_TRIGGER = FILTER_DELAY_REMOVAL_NOT_POSSIBLE_WITH_ANALOG_TRIGGER | ||
| NONBUFFERED_OR_NO_CHANNELS = -201395 | ||
| TRISTATE_LOGIC_LEVEL_NOT_SPECD_FOR_ENTIRE_PORT = -201394 | ||
| TRISTATE_LOGIC_LEVEL_NOT_SUPPORTED_ON_DIG_OUT_CHAN = -201393 | ||
|
|
@@ -343,7 +349,9 @@ class DAQmxErrors(IntEnum): | |
| INVALID_COUPLING_FOR_MEASUREMENT_TYPE = -201279 | ||
| DIGITAL_LINE_UPDATE_TOO_FAST_FOR_DEVICE = -201278 | ||
| CERTIFICATE_IS_TOO_BIG_TO_TRANSFER = -201277 | ||
| ONLY_PEM_OR_DER_CERTIFICATES_ACCEPTED = -201276 | ||
| # Typo in C API; kept for backward compatibility | ||
| ONLY_PEM_OR_DER_CERTITICATES_ACCEPTED = ONLY_PEM_OR_DER_CERTIFICATES_ACCEPTED | ||
| CAL_COUPLING_NOT_SUPPORTED = -201275 | ||
| DEVICE_NOT_SUPPORTED_IN_64_BIT = -201274 | ||
| NETWORK_DEVICE_IN_USE = -201273 | ||
|
|
@@ -758,7 +766,9 @@ class DAQmxErrors(IntEnum): | |
| WHEN_ACQ_COMP_AND_NO_REF_TRIG = -200864 | ||
| WAIT_FOR_NEXT_SAMP_CLK_NOT_SUPPORTED = -200863 | ||
| DEV_IN_UNIDENTIFIED_PXI_CHASSIS = -200862 | ||
| MAX_SOUND_PRESSURE_MIC_SENSITIVITY_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV = -200861 | ||
| # Typo in C API; kept for backward compatibility | ||
| MAX_SOUND_PRESSURE_MIC_SENSITIVIT_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV = MAX_SOUND_PRESSURE_MIC_SENSITIVITY_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV | ||
| MAX_SOUND_PRESSURE_AND_MIC_SENSITIVITY_NOT_SUPPORTED_BY_DEV = -200860 | ||
| AO_BUFFER_SIZE_ZERO_FOR_SAMP_CLK_TIMING_TYPE = -200859 | ||
| AO_CALL_WRITE_BEFORE_START_FOR_SAMP_CLK_TIMING_TYPE = -200858 | ||
|
|
@@ -1290,7 +1300,9 @@ class DAQmxErrors(IntEnum): | |
| DEV_ABSENT_OR_UNAVAILABLE = -200324 | ||
| NO_ADV_TRIG_FOR_MULTI_DEV_SCAN = -200323 | ||
| INTERRUPTS_INSUFFICIENT_DATA_XFER_MECH = -200322 | ||
| INVALID_ATTENUATION_BASED_ON_MIN_MAX = -200321 | ||
| # Typo in C API; kept for backward compatibility | ||
| INVALID_ATTENTUATION_BASED_ON_MIN_MAX = INVALID_ATTENUATION_BASED_ON_MIN_MAX | ||
| CABLED_MODULE_CANNOT_ROUTE_SSH = -200320 | ||
| CABLED_MODULE_CANNOT_ROUTE_CONV_CLK = -200319 | ||
| INVALID_EXCIT_VAL_FOR_SCALING = -200318 | ||
|
|
||
|
Collaborator
There was a problem hiding this comment. This will have to be updated if/when the misspellings are fixed in the internal DAQmx metadata. The substitutions will need to be reversed, so that it adds the misspellings instead of fixing them. Here's another way to fix the misspellings that wouldn't have this problem:
This way, the compatibility items will still exist if/when the misspellings are fixed upstream. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,9 @@ | ||
| <% | ||
| from codegen.utilities.enum_helpers import ERROR_CODE_NAME_SUBSTITUTIONS | ||
| errors_metadata, warnings_metadata = data['DAQmxErrors'], data['DAQmxWarnings'] | ||
|
|
||
| def _fix_name(name): | ||
| return ERROR_CODE_NAME_SUBSTITUTIONS.get(name, name) | ||
| %>\ | ||
| # Do not edit this file; it was automatically generated. | ||
|
|
||
|
|
@@ -10,13 +14,21 @@ __all__ = ['DAQmxErrors', 'DAQmxWarnings'] | |
|
|
||
| class DAQmxErrors(IntEnum): | ||
| %for value in errors_metadata['values']: | ||
| ${_fix_name(value['name'])} = ${value['value']} | ||
| %if value['name'] in ERROR_CODE_NAME_SUBSTITUTIONS: | ||
| # Typo in C API; kept for backward compatibility | ||
| ${value['name']} = ${_fix_name(value['name'])} | ||
| %endif | ||
| %endfor | ||
| UNKNOWN = -1 | ||
|
|
||
|
|
||
| class DAQmxWarnings(IntEnum): | ||
| UNKNOWN = -1 | ||
| %for value in warnings_metadata['values']: | ||
| ${_fix_name(value['name'])} = ${value['value']} | ||
| %if value['name'] in ERROR_CODE_NAME_SUBSTITUTIONS: | ||
| # Typo in C API; kept for backward compatibility | ||
| ${value['name']} = ${_fix_name(value['name'])} | ||
| %endif | ||
| %endfor | ||
|
Collaborator
There was a problem hiding this comment. Since we are not fixing the misspellings in the metadata at this time, please file a bug to the Platform Drivers Venus team about the misspellings in the C and .NET APIs. Thankfully, https://github.com/ni/grpc-device/blob/main/generated/nidaqmx/nidaqmx.proto is not affected because it doesn't include error code constants. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -127,6 +127,16 @@ | |
| "GROUND": "GND", | ||
| } | ||
|
|
||
| # Typos in the C API error code names that are fixed in the Python names | ||
| ERROR_CODE_NAME_SUBSTITUTIONS = { | ||
| "FILTER_DELAY_REMOVAL_NOT_POSSSIBLE_WITH_ANALOG_TRIGGER": "FILTER_DELAY_REMOVAL_NOT_POSSIBLE_WITH_ANALOG_TRIGGER", | ||
| "INVALID_ATTENTUATION_BASED_ON_MIN_MAX": "INVALID_ATTENUATION_BASED_ON_MIN_MAX", | ||
| "INVALIDC_DAQ_SYNC_PORT_CONNECTION_FORMAT": "INVALID_CDAQ_SYNC_PORT_CONNECTION_FORMAT", | ||
| "MAX_SOUND_PRESSURE_MIC_SENSITIVIT_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV": "MAX_SOUND_PRESSURE_MIC_SENSITIVITY_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV", | ||
| "MULTIPLE_SUBSYTEM_CALIBRATION": "MULTIPLE_SUBSYSTEM_CALIBRATION", | ||
| "ONLY_PEM_OR_DER_CERTITICATES_ACCEPTED": "ONLY_PEM_OR_DER_CERTIFICATES_ACCEPTED", | ||
| } | ||
|
|
||
| # bitfield type enums are prefixed with '_'. | ||
| BITFIELD_ENUMS = ["CouplingTypes", "Callback", "TriggerUsageTypes", "Save", "TermCfg"] | ||
|
|
||
|
|
||
|
Collaborator
There was a problem hiding this comment.
In nidaqmx-python, we generally name component/unit tests after the module that we are testing. |
This file contains hidden or 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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,44 @@ | ||
| """Tests for error code renames that fixed typos inherited from the C API.""" | ||
|
|
||
| from __future__ import annotations | ||
|
|
||
| import pytest | ||
|
|
||
| from nidaqmx.error_codes import DAQmxErrors | ||
|
|
||
| # Each tuple is (old_typo_name, new_correct_name). | ||
| _RENAMED_ERROR_CODES = [ | ||
| ( | ||
| "FILTER_DELAY_REMOVAL_NOT_POSSSIBLE_WITH_ANALOG_TRIGGER", | ||
| "FILTER_DELAY_REMOVAL_NOT_POSSIBLE_WITH_ANALOG_TRIGGER", | ||
| ), | ||
| ( | ||
| "INVALID_ATTENTUATION_BASED_ON_MIN_MAX", | ||
| "INVALID_ATTENUATION_BASED_ON_MIN_MAX", | ||
| ), | ||
| ( | ||
| "INVALIDC_DAQ_SYNC_PORT_CONNECTION_FORMAT", | ||
| "INVALID_CDAQ_SYNC_PORT_CONNECTION_FORMAT", | ||
| ), | ||
| ( | ||
| "MAX_SOUND_PRESSURE_MIC_SENSITIVIT_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV", | ||
| "MAX_SOUND_PRESSURE_MIC_SENSITIVITY_RELATED_AI_PROPERTIES_NOT_SUPPORTED_BY_DEV", | ||
| ), | ||
| ( | ||
| "MULTIPLE_SUBSYTEM_CALIBRATION", | ||
| "MULTIPLE_SUBSYSTEM_CALIBRATION", | ||
| ), | ||
| ( | ||
| "ONLY_PEM_OR_DER_CERTITICATES_ACCEPTED", | ||
| "ONLY_PEM_OR_DER_CERTIFICATES_ACCEPTED", | ||
| ), | ||
| ] | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("old_name,new_name", _RENAMED_ERROR_CODES) | ||
|
Collaborator
There was a problem hiding this comment. In this instance, I do not think that parametrization has any advantage over the simple approach: |
||
| def test___renamed_error_code___old_and_new_names___refer_to_same_member( | ||
| old_name: str, new_name: str | ||
| ) -> None: | ||
| old_member = DAQmxErrors[old_name] | ||
| new_member = DAQmxErrors[new_name] | ||
| assert old_member is new_member | ||
Oops, something went wrong.
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.
Including misspelled words like "attentuation" in the dictionary allows us to use the same misspelling elsewhere.
I think it would be better to put the misspelled constant in the dictionary (e.g. "INVALID_ATTENTUATION_BASED_ON_MIN_MAX") so that we catch other instances of the misspelled word.