Skip to content

Fix _add_enum_value_python_name() to respect explicitly set value names #2073

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.

Sign up for GitHub

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

Open
ni-jfitzger wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Fix _add_enum_value_python_name() to respect explicitly set value names #2073

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
bdaa4ad
Add failing enum value build tests
ni-jfitzger Jan 26, 2025
877be66
Fix _add_enum_value_python_name
ni-jfitzger Jan 26, 2025
64f00ea
Override some metadata to prevent API changes
ni-jfitzger Jan 26, 2025
bf0540b
Rewrite change to use a temporary key, '_python_name', for processing.
ni-jfitzger Jan 26, 2025
ff725c9
Tweak: don't skip prefix and suffix removal just because there's only…
ni-jfitzger Jan 26, 2025
701c364
No need to use python_name for last remaining value
ni-jfitzger Jan 26, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 13 additions & 2 deletions build/helper/metadata_add_all.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -609,10 +609,11 @@ def _get_least_restrictive_codegen_method(codegen_methods):
def _add_enum_value_python_name(enum_info, config):
'''Add 'python_name' for all values, removing any common prefixes and suffixes'''
for v in enum_info['values']:
v['user_set_python_name'] = 'python_name' in v
Comment thread
ni-jfitzger marked this conversation as resolved.
Outdated
if 'python_name' not in v:
v['python_name'] = v['name'].replace('{}_VAL_'.format(config['module_name'].upper()), '')

# We are using an os.path function do find any common prefix. So that we don't
# We are using an os.path function to find any common prefix. So that we don't
# get 'O' in 'ON' and 'OFF' we remove characters at the end until they are '_'
names = [v['python_name'] for v in enum_info['values']]
prefix = os.path.commonprefix(names)
Expand All @@ -627,13 +628,20 @@ def _add_enum_value_python_name(enum_info, config):
# '_' only means the name starts with a number
if len(prefix) > 0 and prefix != '_':
for v in enum_info['values']:
if v['user_set_python_name']:
continue
assert v['python_name'].startswith(prefix), '{} does not start with {}'.format(v['name'], prefix)
v['prefix'] = prefix
v['python_name'] = v['python_name'].replace(prefix, '')

# Now we need to look for common suffixes
# Using the slow method of reversing a string for readability
rev_names = [''.join(reversed(v['python_name'])) for v in enum_info['values']]
# We do not include hardcoded python names when looking for common suffixes
Copy link
Copy Markdown
Member

@marcoskirsch marcoskirsch Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you use hardcoded as a synonym to user_set. Reduce ambiguity by using one term to refer to one concept everywhere.

Copy link
Copy Markdown
Member

@marcoskirsch marcoskirsch Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW I like the word "explicit" for this. It means (in my mind) that it's there written in metadata.
"User set" is a bit ambiguous because it's not super clear who the user is.

Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've stopped using the word hardcoded and made use of the word explicit, instead, as part of my rewrite.
I eliminated the user_set_python_name key. We'll do processing on the temporary key, '_python_name', instead, so we won't need a special key to track whether we should expand the name.

rev_names = [
''.join(reversed(v['python_name']))
for v in enum_info['values']
if not v['user_set_python_name']
]
suffix = os.path.commonprefix(rev_names)
while len(suffix) > 0 and suffix[-1] != '_':
suffix = suffix[:-1]
Expand All @@ -649,12 +657,15 @@ def _add_enum_value_python_name(enum_info, config):
# '_' only means the name starts with a number
if len(suffix) > 0:
for v in enum_info['values']:
if v['user_set_python_name']:
continue
assert v['python_name'].endswith(suffix), '{} does not end with {}'.format(v['name'], suffix)
v['suffix'] = suffix
v['python_name'] = v['python_name'][:-len(suffix)]

# We need to check again to see if we have any values that start with a digit
# If we are not going to code generate this enum, we don't care about this
# Even hardcoded names should follow this rule
for v in enum_info['values']:
assert v['python_name'], enum_info
if enum_info['codegen_method'] != 'no' and v['python_name'][0].isdigit():
Expand Down
99 changes: 91 additions & 8 deletions build/unit_tests/test_metadata_add_all.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -870,6 +870,57 @@ def _compare_dicts(actual, expected):
}
]
},
'EnumWithCommonPrefixInValueNames': {
'codegen_method': 'public',
'values': [
{
'name': 'COLOR_BRIGHT_RED',
'value': 1
},
{
'name': 'COLOR_BRIGHT_BLUE',
'value': 2
}
]
},
'EnumWithHardcodedValueNames': {
'codegen_method': 'public',
'values': [
{
'name': 'THE_COLOR_RED',
'python_name': 'COLOR_DARK_RED',
'value': 1
},
{
'name': 'THE_COLOR_BLUE',
'python_name': 'COLOR_DARK_BLUE',
'value': 2
}
]
},
'EnumWithHardcodedValueNamesMixedIn': {
'codegen_method': 'public',
'values': [
{
'name': 'DISTANCE_MILES',
'value': 1
},
{
'name': 'DISTANCE_KILOMETERS',
'python_name': 'DISTANCE_KILOMETERS',
'value': 2
},
{
'name': 'DISTANCE_METERS',
'python_name': 'DISTANCE_METERS',
'value': 5
},
{
'name': 'DISTANCE_YARDS',
'value': 42
}
]
},
}


Expand All @@ -878,10 +929,10 @@ def _compare_dicts(actual, expected):
'codegen_method': 'no',
'python_name': 'Color',
'values': [
{'documentation': {'description': 'Like blood.'}, 'name': 'RED', 'value': 1, 'python_name': 'RED'},
{'documentation': {'description': 'Like the sky.'}, 'name': 'BLUE', 'value': 2, 'python_name': 'BLUE'},
{'documentation': {'description': 'Like a banana.'}, 'name': 'YELLOW', 'value': 2, 'python_name': 'YELLOW'},
{'documentation': {'description': "Like this developer's conscience."}, 'name': 'BLACK', 'value': 2, 'python_name': 'BLACK'}
{'documentation': {'description': 'Like blood.'}, 'name': 'RED', 'value': 1, 'python_name': 'RED', 'user_set_python_name': False},
{'documentation': {'description': 'Like the sky.'}, 'name': 'BLUE', 'value': 2, 'python_name': 'BLUE', 'user_set_python_name': False},
{'documentation': {'description': 'Like a banana.'}, 'name': 'YELLOW', 'value': 2, 'python_name': 'YELLOW', 'user_set_python_name': False},
{'documentation': {'description': "Like this developer's conscience."}, 'name': 'BLACK', 'value': 2, 'python_name': 'BLACK', 'user_set_python_name': False}
]
},
'EnumWithConverter': {
Expand All @@ -890,10 +941,36 @@ def _compare_dicts(actual, expected):
'converted_value_to_enum_function_name': 'convert_to_enum_with_converter_enum',
'enum_to_converted_value_function_name': 'convert_from_enum_with_converter_enum',
'values': [
{'name': 'RED', 'value': 1, 'converts_to_value': True, 'python_name': 'RED'},
{'name': 'BLUE', 'value': 2, 'converts_to_value': False, 'python_name': 'BLUE'},
{'name': 'YELLOW', 'value': 5, 'converts_to_value': 'yellow', 'python_name': 'YELLOW'},
{'name': 'BLACK', 'value': 42, 'converts_to_value': 42, 'python_name': 'BLACK'}
{'name': 'RED', 'value': 1, 'converts_to_value': True, 'python_name': 'RED', 'user_set_python_name': False},
{'name': 'BLUE', 'value': 2, 'converts_to_value': False, 'python_name': 'BLUE', 'user_set_python_name': False},
{'name': 'YELLOW', 'value': 5, 'converts_to_value': 'yellow', 'python_name': 'YELLOW', 'user_set_python_name': False},
{'name': 'BLACK', 'value': 42, 'converts_to_value': 42, 'python_name': 'BLACK', 'user_set_python_name': False}
]
},
'EnumWithCommonPrefixInValueNames': {
'codegen_method': 'public',
'python_name': 'EnumWithCommonPrefixInValueNames',
'values': [
{'name': 'COLOR_BRIGHT_RED', 'value': 1, 'python_name': 'RED', 'user_set_python_name': False, 'prefix': 'COLOR_BRIGHT_'},
{'name': 'COLOR_BRIGHT_BLUE', 'value': 2, 'python_name': 'BLUE', 'user_set_python_name': False, 'prefix': 'COLOR_BRIGHT_'}
]
},
'EnumWithHardcodedValueNames': {
'codegen_method': 'public',
'python_name': 'EnumWithHardcodedValueNames',
'values': [
{'name': 'THE_COLOR_RED', 'value': 1, 'python_name': 'COLOR_DARK_RED', 'user_set_python_name': True},
{'name': 'THE_COLOR_BLUE', 'value': 2, 'python_name': 'COLOR_DARK_BLUE', 'user_set_python_name': True}
]
},
'EnumWithHardcodedValueNamesMixedIn': {
'codegen_method': 'public',
'python_name': 'EnumWithHardcodedValueNamesMixedIn',
'values': [
{'name': 'DISTANCE_MILES', 'value': 1, 'python_name': 'MILES', 'user_set_python_name': False, 'prefix': 'DISTANCE_'},
{'name': 'DISTANCE_KILOMETERS', 'value': 2, 'python_name': 'DISTANCE_KILOMETERS', 'user_set_python_name': True},
{'name': 'DISTANCE_METERS', 'value': 5, 'python_name': 'DISTANCE_METERS', 'user_set_python_name': True},
{'name': 'DISTANCE_YARDS', 'value': 42, 'python_name': 'YARDS', 'user_set_python_name': False, 'prefix': 'DISTANCE_'}
]
},
}
Expand Down Expand Up @@ -1113,6 +1190,9 @@ def test_get_functions_that_use_enums():
expected_output = {
'Color': ['PythonOnlyMethod'],
'EnumWithConverter': ['PublicMethod', 'PrivateMethod'],
'EnumWithCommonPrefixInValueNames': [],
'EnumWithHardcodedValueNames': [],
'EnumWithHardcodedValueNamesMixedIn': [],
}
actual_output = _get_functions_that_use_enums(actual_enums, actual_config)
_compare_dicts(actual_output, expected_output)
Expand All @@ -1123,6 +1203,9 @@ def test_get_attributes_that_use_enums():
expected_output = {
'Color': ['1000002'],
'EnumWithConverter': ['1000001', '1000003'],
'EnumWithCommonPrefixInValueNames': [],
'EnumWithHardcodedValueNames': [],
'EnumWithHardcodedValueNamesMixedIn': [],
}
actual_output = _get_attributes_that_use_enums(actual_enums, actual_config)
_compare_dicts(actual_output, expected_output)
Expand Down
125 changes: 125 additions & 0 deletions src/nidcpower/metadata/enums_addon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,130 @@
# Any changes to the API should be made here. enums.py is code generated

enums_override_metadata = {
# TODO (ni-jfitzger): delete this override once python_name is corrected for each value. See https://github.com/ni/nimi-python/issues/2072
'ComplianceLimitSymmetry': {
'values': [
{
'documentation': {
'description': 'Compliance limits are specified symmetrically about 0.'
},
'name': 'NIDCPOWER_VAL_COMPLIANCE_LIMIT_SYMMETRY_SYMMETRIC',
'value': 0
},
{
'documentation': {
'description': 'Compliance limits can be specified asymmetrically with respect to 0.'
},
'name': 'NIDCPOWER_VAL_COMPLIANCE_LIMIT_SYMMETRY_ASYMMETRIC',
'value': 1
}
]
},
# TODO (ni-jfitzger): delete this override once python_name is corrected for each value. See https://github.com/ni/nimi-python/issues/2072
'Event': {
'values': [
{
'documentation': {
'description': 'Specifies the Source Complete event.'
},
'name': 'NIDCPOWER_VAL_SOURCE_COMPLETE_EVENT',
'python_name': 'SOURCE_COMPLETE',
'value': 1030
},
{
'documentation': {
'description': 'Specifies the Measure Complete event.'
},
'name': 'NIDCPOWER_VAL_MEASURE_COMPLETE_EVENT',
'python_name': 'MEASURE_COMPLETE',
'value': 1031
},
{
'documentation': {
'description': 'Specifies the Sequence Iteration Complete event.'
},
'name': 'NIDCPOWER_VAL_SEQUENCE_ITERATION_COMPLETE_EVENT',
'python_name': 'SEQUENCE_ITERATION_COMPLETE',
'value': 1032
},
{
'documentation': {
'description': 'Specifies the Sequence Engine Done event.'
},
'name': 'NIDCPOWER_VAL_SEQUENCE_ENGINE_DONE_EVENT',
'python_name': 'SEQUENCE_ENGINE_DONE',
'value': 1033
},
{
'documentation': {
'description': 'Specifies the Pulse Complete event.'
},
'name': 'NIDCPOWER_VAL_PULSE_COMPLETE_EVENT',
'python_name': 'PULSE_COMPLETE',
'value': 1051
},
{
'documentation': {
'description': 'Specifies the Ready for Pulse Trigger event.'
},
'name': 'NIDCPOWER_VAL_READY_FOR_PULSE_TRIGGER_EVENT',
'python_name': 'READY_FOR_PULSE_TRIGGER',
'value': 1052
}
]
},
# TODO (ni-jfitzger): delete this override once python_name is corrected for each value. See https://github.com/ni/nimi-python/issues/2072
'SendSoftwareEdgeTriggerType': {
'values': [
{
'documentation': {
'description': 'Asserts the Start trigger.'
},
'name': 'NIDCPOWER_VAL_START_TRIGGER',
'python_name': 'START',
'value': 1034
},
{
'documentation': {
'description': 'Asserts the Source trigger.'
},
'name': 'NIDCPOWER_VAL_SOURCE_TRIGGER',
'python_name': 'SOURCE',
'value': 1035
},
{
'documentation': {
'description': 'Asserts the Measure trigger.'
},
'name': 'NIDCPOWER_VAL_MEASURE_TRIGGER',
'python_name': 'MEASURE',
'value': 1036
},
{
'documentation': {
'description': 'Asserts the Sequence Advance trigger.'
},
'name': 'NIDCPOWER_VAL_SEQUENCE_ADVANCE_TRIGGER',
'python_name': 'SEQUENCE_ADVANCE',
'value': 1037
},
{
'documentation': {
'description': 'Asserts the Pulse trigger.'
},
'name': 'NIDCPOWER_VAL_PULSE_TRIGGER',
'python_name': 'PULSE',
'value': 1053
},
{
'documentation': {
'description': 'Asserts the Shutdown trigger.'
},
'name': 'NIDCPOWER_VAL_SHUTDOWN_TRIGGER',
'python_name': 'SHUTDOWN',
'value': 1118
}
]
},
}

37 changes: 37 additions & 0 deletions src/nidmm/metadata/enums_addon.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,5 +2,42 @@
# Any changes to the API should be made here. enums.py is code generated

enums_override_metadata = {
# TODO (ni-jfitzger): delete this override once python_name is corrected for each value. See https://github.com/ni/nimi-python/issues/2072
'ThermistorType': {
'values': [
{
'documentation': {
'description': 'Custom'
},
'name': 'NIDMM_VAL_TEMP_THERMISTOR_CUSTOM',
'python_name': 'CUSTOM',
Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strangely, (with the codegen helper changes) if I set python_name for all the values except this one, this one came back as TEMP. I'm not sure why it gets cut off like that, but didn't feel it was worth further investigation.

Copy link
Copy Markdown
Member

@marcoskirsch marcoskirsch Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does that mean that if a client wants to explicitly provide the python name for a subset of enum values, we don't know what will be the expanded value here?

I feel like it's worth getting to the bottom of, and having a test case for.

Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was almost certainly due to common suffix removal.

The behavior was a little bit hard to follow, so I've rewritten my change and instead of using a boolean key for tracking whether to touch something, I'll have us do all of the processsing with a temporary key, '_python_name'.

I've also added another unit test enum related to this. There's a peculiar behavior with common prefix and common suffix removal when you only have one value to calculate the prefix or suffix from. It calculates the prefix or suffix as the entire string and we then remove almost that entire string (upto and including the first/last underscore). I setting prefix and suffix to '', instead, when there's less than 2 values to calculate the commmon prefix or suffix from, but (upsetting as it is) we have one or two enums with a single enum value that actually make use of this behavior. I can still change it if you want, though. It seems like a footgun.

Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 26, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the latest change, we no longer need to set 'python_name': 'CUSTOM', but this behavior still seems like a footgun.

Copy link
Copy Markdown
Member

@marcoskirsch marcoskirsch Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the behavior observed when only a subset of the enum values has python_name explicitly in metadata is hard to describle / footgun, we could consider disallowing this altogether and forcing clients to specify none or all. I think the case is rare enough that there would be very few cases of this.

Counterpoint is that bad expansion of a subset of enum values should be fairly apparent in generated code diffs.

What do you think?

Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's easy to describe at this point. Everything up through the last underscore is trimmed. This is the same behavior we would see if the values that use python_name didn't exist.

You're correct that is very rare and even rarer is the case where we have an enum with only one value and the one value has multiple words and that enum is used somewhere so we don't filter it out in codegen. We only seem to have a couple of those and the current value names seem okay.

I think it's fine to leave this change as is. We should be able to catch any issues in review.

Copy link
Copy Markdown
Collaborator Author

@ni-jfitzger ni-jfitzger Jan 27, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could consider disallowing this altogether and forcing clients to specify none or all

Yeah, that's another option. Wouldn't be too difficult to do, though I'm not sure how easy it is to test.

'value': 0
},
{
'documentation': {
'description': '44004'
},
'name': 'NIDMM_VAL_TEMP_THERMISTOR_44004',
'python_name': 'THERMISTOR_44004',
'value': 1
},
{
'documentation': {
'description': '44006'
},
'name': 'NIDMM_VAL_TEMP_THERMISTOR_44006',
'python_name': 'THERMISTOR_44006',
'value': 2
},
{
'documentation': {
'description': '44007'
},
'name': 'NIDMM_VAL_TEMP_THERMISTOR_44007',
'python_name': 'THERMISTOR_44007',
'value': 3
}
]
},
}