From bdaa4ad557ffeea9b9770e7cc81212fe67a87196 Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 10:54:54 -0600 Subject: [PATCH 1/6] Add failing enum value build tests --- build/unit_tests/test_metadata_add_all.py | 85 ++++++++++++++++++++++- 1 file changed, 84 insertions(+), 1 deletion(-) diff --git a/build/unit_tests/test_metadata_add_all.py b/build/unit_tests/test_metadata_add_all.py index 67a5750a4..9a7a73a35 100644 --- a/build/unit_tests/test_metadata_add_all.py +++ b/build/unit_tests/test_metadata_add_all.py @@ -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 + } + ] + }, } @@ -893,7 +944,33 @@ def _compare_dicts(actual, expected): {'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': 'BLACK', 'value': 42, 'converts_to_value': 42, 'python_name': 'BLACK', } + ] + }, + 'EnumWithCommonPrefixInValueNames': { + 'codegen_method': 'public', + 'python_name': 'EnumWithCommonPrefixInValueNames', + 'values': [ + {'name': 'COLOR_BRIGHT_RED', 'value': 1, 'python_name': 'RED', 'prefix': 'COLOR_BRIGHT_'}, + {'name': 'COLOR_BRIGHT_BLUE', 'value': 2, 'python_name': 'BLUE', 'prefix': 'COLOR_BRIGHT_'} + ] + }, + 'EnumWithHardcodedValueNames': { + 'codegen_method': 'public', + 'python_name': 'EnumWithHardcodedValueNames', + 'values': [ + {'name': 'THE_COLOR_RED', 'value': 1, 'python_name': 'COLOR_DARK_RED'}, + {'name': 'THE_COLOR_BLUE', 'value': 2, 'python_name': 'COLOR_DARK_BLUE'} + ] + }, + 'EnumWithHardcodedValueNamesMixedIn': { + 'codegen_method': 'public', + 'python_name': 'EnumWithHardcodedValueNamesMixedIn', + 'values': [ + {'name': 'DISTANCE_MILES', 'value': 1, 'python_name': 'MILES', 'prefix': 'DISTANCE_'}, + {'name': 'DISTANCE_KILOMETERS', 'value': 2, 'python_name': 'DISTANCE_KILOMETERS'}, + {'name': 'DISTANCE_METERS', 'value': 5, 'python_name': 'DISTANCE_METERS'}, + {'name': 'DISTANCE_YARDS', 'value': 42, 'python_name': 'YARDS', 'prefix': 'DISTANCE_'} ] }, } @@ -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) @@ -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) From 877be66b35c666854fe01fe622c59973b8e91423 Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 11:03:10 -0600 Subject: [PATCH 2/6] Fix _add_enum_value_python_name --- build/helper/metadata_add_all.py | 15 +++++++++-- build/unit_tests/test_metadata_add_all.py | 32 +++++++++++------------ 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/build/helper/metadata_add_all.py b/build/helper/metadata_add_all.py index ed9d836b6..430f33ba7 100644 --- a/build/helper/metadata_add_all.py +++ b/build/helper/metadata_add_all.py @@ -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 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) @@ -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 + 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] @@ -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(): diff --git a/build/unit_tests/test_metadata_add_all.py b/build/unit_tests/test_metadata_add_all.py index 9a7a73a35..6156bfc75 100644 --- a/build/unit_tests/test_metadata_add_all.py +++ b/build/unit_tests/test_metadata_add_all.py @@ -929,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': { @@ -941,36 +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', 'prefix': 'COLOR_BRIGHT_'}, - {'name': 'COLOR_BRIGHT_BLUE', 'value': 2, 'python_name': 'BLUE', 'prefix': 'COLOR_BRIGHT_'} + {'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'}, - {'name': 'THE_COLOR_BLUE', 'value': 2, 'python_name': 'COLOR_DARK_BLUE'} + {'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', 'prefix': 'DISTANCE_'}, - {'name': 'DISTANCE_KILOMETERS', 'value': 2, 'python_name': 'DISTANCE_KILOMETERS'}, - {'name': 'DISTANCE_METERS', 'value': 5, 'python_name': 'DISTANCE_METERS'}, - {'name': 'DISTANCE_YARDS', 'value': 42, 'python_name': 'YARDS', 'prefix': 'DISTANCE_'} + {'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_'} ] }, } From 64f00ea73b5d119c2457d9dd7b9e92399a26d49e Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 12:16:07 -0600 Subject: [PATCH 3/6] Override some metadata to prevent API changes --- src/nidcpower/metadata/enums_addon.py | 125 ++++++++++++++++++++++++++ src/nidmm/metadata/enums_addon.py | 37 ++++++++ 2 files changed, 162 insertions(+) diff --git a/src/nidcpower/metadata/enums_addon.py b/src/nidcpower/metadata/enums_addon.py index 2ecd181b7..4be6136d1 100644 --- a/src/nidcpower/metadata/enums_addon.py +++ b/src/nidcpower/metadata/enums_addon.py @@ -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 + } + ] + }, } diff --git a/src/nidmm/metadata/enums_addon.py b/src/nidmm/metadata/enums_addon.py index 2ecd181b7..48798760b 100644 --- a/src/nidmm/metadata/enums_addon.py +++ b/src/nidmm/metadata/enums_addon.py @@ -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', + '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 + } + ] + }, } From bf0540b312e83998feab610156056cb410948d41 Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 16:51:03 -0600 Subject: [PATCH 4/6] Rewrite change to use a temporary key, '_python_name', for processing. Add another unit test --- build/helper/metadata_add_all.py | 52 ++++++++------ build/unit_tests/test_metadata_add_all.py | 82 ++++++++++++++++------- 2 files changed, 89 insertions(+), 45 deletions(-) diff --git a/build/helper/metadata_add_all.py b/build/helper/metadata_add_all.py index 430f33ba7..317ec9fe7 100644 --- a/build/helper/metadata_add_all.py +++ b/build/helper/metadata_add_all.py @@ -609,16 +609,21 @@ 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 + # Some values have an explicitly set python_name. + # To avoid altering these, we'll use _python_name for all of our processing. if 'python_name' not in v: - v['python_name'] = v['name'].replace('{}_VAL_'.format(config['module_name'].upper()), '') + v['_python_name'] = v['name'].replace('{}_VAL_'.format(config['module_name'].upper()), '') # 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) - while len(prefix) > 0 and prefix[-1] != '_': - prefix = prefix[:-1] + # Exclude explicitly set names from the prefix calculation + names = [v['_python_name'] for v in enum_info['values'] if '_python_name' in v] + if len(names) < 2: + prefix = '' + else: + prefix = os.path.commonprefix(names) + while len(prefix) > 0 and prefix[-1] != '_': + prefix = prefix[:-1] # If the prefix is in the whitelist, we don't want to remove it so set to empty string if 'enum_whitelist_prefix' in config and prefix in config['enum_whitelist_prefix']: @@ -628,23 +633,26 @@ 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']: + if 'python_name' in v: continue - assert v['python_name'].startswith(prefix), '{} does not start with {}'.format(v['name'], prefix) + 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, '') + 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 - # We do not include hardcoded python names when looking for common suffixes + # We exclude explicitly set names when looking for common suffixes rev_names = [ - ''.join(reversed(v['python_name'])) + ''.join(reversed(v['_python_name'])) for v in enum_info['values'] - if not v['user_set_python_name'] + if '_python_name' in v ] - suffix = os.path.commonprefix(rev_names) - while len(suffix) > 0 and suffix[-1] != '_': - suffix = suffix[:-1] + if len(rev_names) < 2: + suffix = '' + else: + suffix = os.path.commonprefix(rev_names) + while len(suffix) > 0 and suffix[-1] != '_': + suffix = suffix[:-1] # Unreverse the suffix suffix = ''.join(reversed(suffix)) @@ -657,15 +665,21 @@ 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']: + if 'python_name' in v: continue - assert v['python_name'].endswith(suffix), '{} does not end with {}'.format(v['name'], suffix) + 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)] + v['_python_name'] = v['_python_name'][:-len(suffix)] + + for v in enum_info['values']: + if 'python_name' in v: + continue + v['python_name'] = v['_python_name'] + del v['_python_name'] # 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 + # All 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(): diff --git a/build/unit_tests/test_metadata_add_all.py b/build/unit_tests/test_metadata_add_all.py index 6156bfc75..7109c1055 100644 --- a/build/unit_tests/test_metadata_add_all.py +++ b/build/unit_tests/test_metadata_add_all.py @@ -883,7 +883,7 @@ def _compare_dicts(actual, expected): } ] }, - 'EnumWithHardcodedValueNames': { + 'EnumWithExplicitValueNames': { 'codegen_method': 'public', 'values': [ { @@ -898,7 +898,7 @@ def _compare_dicts(actual, expected): } ] }, - 'EnumWithHardcodedValueNamesMixedIn': { + 'EnumWithExplicitValueNamesMixedIn': { 'codegen_method': 'public', 'values': [ { @@ -921,6 +921,25 @@ def _compare_dicts(actual, expected): } ] }, + 'EnumWithOneExpandedValueAndCommonPrefixSuffix': { + 'codegen_method': 'public', + 'values': [ + { + 'name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX', + 'value': 1 + }, + { + 'name': 'COMMON_PREFIX_BASEBALL', + 'python_name': 'BASEBALL', # we want to test that this excludes it from common prefix/suffix calculations + 'value': 2 + }, + { + 'name': 'BASKETBALL_COMMON_SUFFIX', + 'python_name': 'BASKETBALL', # we want to test that this excludes it from common prefix/suffix calculations + 'value': 3 + }, + ] + }, } @@ -929,10 +948,10 @@ def _compare_dicts(actual, expected): 'codegen_method': 'no', 'python_name': 'Color', 'values': [ - {'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} + {'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'} ] }, 'EnumWithConverter': { @@ -941,36 +960,45 @@ 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', '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} + {'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'} ] }, '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_'} + {'name': 'COLOR_BRIGHT_RED', 'value': 1, 'python_name': 'RED', 'prefix': 'COLOR_BRIGHT_'}, + {'name': 'COLOR_BRIGHT_BLUE', 'value': 2, 'python_name': 'BLUE', 'prefix': 'COLOR_BRIGHT_'} + ] + }, + 'EnumWithExplicitValueNames': { + 'codegen_method': 'public', + 'python_name': 'EnumWithExplicitValueNames', + 'values': [ + {'name': 'THE_COLOR_RED', 'value': 1, 'python_name': 'COLOR_DARK_RED'}, + {'name': 'THE_COLOR_BLUE', 'value': 2, 'python_name': 'COLOR_DARK_BLUE'} ] }, - 'EnumWithHardcodedValueNames': { + 'EnumWithExplicitValueNamesMixedIn': { 'codegen_method': 'public', - 'python_name': 'EnumWithHardcodedValueNames', + 'python_name': 'EnumWithExplicitValueNamesMixedIn', '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} + {'name': 'DISTANCE_MILES', 'value': 1, 'python_name': 'MILES', 'prefix': 'DISTANCE_'}, + {'name': 'DISTANCE_KILOMETERS', 'value': 2, 'python_name': 'DISTANCE_KILOMETERS'}, # explicitly set + {'name': 'DISTANCE_METERS', 'value': 5, 'python_name': 'DISTANCE_METERS'}, # explicitly set + {'name': 'DISTANCE_YARDS', 'value': 42, 'python_name': 'YARDS', 'prefix': 'DISTANCE_'} ] }, - 'EnumWithHardcodedValueNamesMixedIn': { + 'EnumWithOneExpandedValueAndCommonPrefixSuffix': { 'codegen_method': 'public', - 'python_name': 'EnumWithHardcodedValueNamesMixedIn', + 'python_name': 'EnumWithOneExpandedValueAndCommonPrefixSuffix', '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_'} + {'name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX', 'value': 1, 'python_name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX'}, + {'name': 'COMMON_PREFIX_BASEBALL', 'value': 2, 'python_name': 'BASEBALL'}, # explicitly set + {'name': 'BASKETBALL_COMMON_SUFFIX', 'value': 3, 'python_name': 'BASKETBALL'}, # explicitly set ] }, } @@ -1191,8 +1219,9 @@ def test_get_functions_that_use_enums(): 'Color': ['PythonOnlyMethod'], 'EnumWithConverter': ['PublicMethod', 'PrivateMethod'], 'EnumWithCommonPrefixInValueNames': [], - 'EnumWithHardcodedValueNames': [], - 'EnumWithHardcodedValueNamesMixedIn': [], + 'EnumWithExplicitValueNames': [], + 'EnumWithExplicitValueNamesMixedIn': [], + 'EnumWithOneExpandedValueAndCommonPrefixSuffix': [], } actual_output = _get_functions_that_use_enums(actual_enums, actual_config) _compare_dicts(actual_output, expected_output) @@ -1204,8 +1233,9 @@ def test_get_attributes_that_use_enums(): 'Color': ['1000002'], 'EnumWithConverter': ['1000001', '1000003'], 'EnumWithCommonPrefixInValueNames': [], - 'EnumWithHardcodedValueNames': [], - 'EnumWithHardcodedValueNamesMixedIn': [], + 'EnumWithExplicitValueNames': [], + 'EnumWithExplicitValueNamesMixedIn': [], + 'EnumWithOneExpandedValueAndCommonPrefixSuffix': [], } actual_output = _get_attributes_that_use_enums(actual_enums, actual_config) _compare_dicts(actual_output, expected_output) From ff725c952ec86f568a7e45793a1b25c00c15f336 Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 17:06:46 -0600 Subject: [PATCH 5/6] Tweak: don't skip prefix and suffix removal just because there's only one value. There are existing enums with only one value that rely on this removal, as weird and questionable as that seems to me. --- build/helper/metadata_add_all.py | 18 ++++++------------ build/unit_tests/test_metadata_add_all.py | 2 +- 2 files changed, 7 insertions(+), 13 deletions(-) diff --git a/build/helper/metadata_add_all.py b/build/helper/metadata_add_all.py index 317ec9fe7..7aedd29ce 100644 --- a/build/helper/metadata_add_all.py +++ b/build/helper/metadata_add_all.py @@ -618,12 +618,9 @@ def _add_enum_value_python_name(enum_info, config): # get 'O' in 'ON' and 'OFF' we remove characters at the end until they are '_' # Exclude explicitly set names from the prefix calculation names = [v['_python_name'] for v in enum_info['values'] if '_python_name' in v] - if len(names) < 2: - prefix = '' - else: - prefix = os.path.commonprefix(names) - while len(prefix) > 0 and prefix[-1] != '_': - prefix = prefix[:-1] + prefix = os.path.commonprefix(names) + while len(prefix) > 0 and prefix[-1] != '_': + prefix = prefix[:-1] # If the prefix is in the whitelist, we don't want to remove it so set to empty string if 'enum_whitelist_prefix' in config and prefix in config['enum_whitelist_prefix']: @@ -647,12 +644,9 @@ def _add_enum_value_python_name(enum_info, config): for v in enum_info['values'] if '_python_name' in v ] - if len(rev_names) < 2: - suffix = '' - else: - suffix = os.path.commonprefix(rev_names) - while len(suffix) > 0 and suffix[-1] != '_': - suffix = suffix[:-1] + suffix = os.path.commonprefix(rev_names) + while len(suffix) > 0 and suffix[-1] != '_': + suffix = suffix[:-1] # Unreverse the suffix suffix = ''.join(reversed(suffix)) diff --git a/build/unit_tests/test_metadata_add_all.py b/build/unit_tests/test_metadata_add_all.py index 7109c1055..7d123a47d 100644 --- a/build/unit_tests/test_metadata_add_all.py +++ b/build/unit_tests/test_metadata_add_all.py @@ -996,7 +996,7 @@ def _compare_dicts(actual, expected): 'codegen_method': 'public', 'python_name': 'EnumWithOneExpandedValueAndCommonPrefixSuffix', 'values': [ - {'name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX', 'value': 1, 'python_name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX'}, + {'name': 'COMMON_PREFIX_FOOTBALL_COMMON_SUFFIX', 'value': 1, 'python_name': 'SUFFIX', 'prefix': 'COMMON_PREFIX_FOOTBALL_COMMON_'}, {'name': 'COMMON_PREFIX_BASEBALL', 'value': 2, 'python_name': 'BASEBALL'}, # explicitly set {'name': 'BASKETBALL_COMMON_SUFFIX', 'value': 3, 'python_name': 'BASKETBALL'}, # explicitly set ] From 701c3644268d5db51163e12f54f1dc5894bc82ae Mon Sep 17 00:00:00 2001 From: Jay Fitzgerald <34140133+ni-jfitzger@users.noreply.github.com> Date: Sun, 26 Jan 2025 17:21:04 -0600 Subject: [PATCH 6/6] No need to use python_name for last remaining value --- src/nidmm/metadata/enums_addon.py | 1 - 1 file changed, 1 deletion(-) diff --git a/src/nidmm/metadata/enums_addon.py b/src/nidmm/metadata/enums_addon.py index 48798760b..b216a3b26 100644 --- a/src/nidmm/metadata/enums_addon.py +++ b/src/nidmm/metadata/enums_addon.py @@ -10,7 +10,6 @@ 'description': 'Custom' }, 'name': 'NIDMM_VAL_TEMP_THERMISTOR_CUSTOM', - 'python_name': 'CUSTOM', 'value': 0 }, {