Fix _add_enum_value_python_name() to respect explicitly set value names #2073
Conversation
src/nidmm/metadata/enums_addon.py
Outdated
| 'description': 'Custom' | ||
| }, | ||
| 'name': 'NIDMM_VAL_TEMP_THERMISTOR_CUSTOM', | ||
| 'python_name': 'CUSTOM', |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
With the latest change, we no longer need to set 'python_name': 'CUSTOM', but this behavior still seems like a footgun.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #2073 +/- ##
==========================================
+ Coverage 91.33% 91.37% +0.03%
==========================================
Files 66 66
Lines 16274 16283 +9
==========================================
+ Hits 14864 14878 +14
+ Misses 1410 1405 -5
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
build/helper/metadata_add_all.py
Outdated
| 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 |
There was a problem hiding this comment.
Here you use hardcoded as a synonym to user_set. Reduce ambiguity by using one term to refer to one concept everywhere.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
src/nidmm/metadata/enums_addon.py
Outdated
| 'description': 'Custom' | ||
| }, | ||
| 'name': 'NIDMM_VAL_TEMP_THERMISTOR_CUSTOM', | ||
| 'python_name': 'CUSTOM', |
There was a problem hiding this comment.
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.
Add another unit test
… one value. There are existing enums with only one value that rely on this removal, as weird and questionable as that seems to me.
ni-jfitzger
changed the title
[ ] I've updated CHANGELOG.md if applicable.What does this Pull Request accomplish?
The
python_namemetadata key is supposed to allow us to explicitly set the name that will be used by the Python API. For enum values, this was not being respected.This change fixes that and adds additional unit test coverage for enum value expansion.
As part of this, we have to temporarily override some enum value metadata. Followup item: #2072
List issues fixed by this Pull Request below, if any.
What testing has been done?