Prepend a prefix to field names to allow use as Python identifiers #206
Conversation
Codecov Report
@@ Coverage Diff @@
## main #206 +/- ##
========================================
Coverage ? 100.00%
========================================
Files ? 41
Lines ? 1299
Branches ? 0
========================================
Hits ? 1299
Misses ? 0
Partials ? 0
Continue to review full report at Codecov.
|
kalzoo
force-pushed
the
203-invalid-identifiers
branch
from
53aced9 to
0bb503f
Compare
openapi_python_client/utils.py
Outdated
| if value.isidentifier(): | ||
| return value | ||
|
|
||
| new_value = f"field_{value}" |
There was a problem hiding this comment.
note: as a user I'd prefer this to be configurable, but didn't notice a simple path for a Config to get here
There was a problem hiding this comment.
Yeah config right now is pretty icky- bunch of global mutations. I'll fix it eventually 😅
openapi_python_client/utils.py
Outdated
| if new_value.isidentifier(): | ||
| return new_value | ||
|
|
||
| raise ValueError(f"Cannot convert {value} to a valid python identifier") |
There was a problem hiding this comment.
We can get craftier here and replace other invalid characters within the string as well
|
@kalzoo thanks for the PR, I'll check it out a bit later! I just wanted to pop in to let you know to expect CircleCI to fail on Python 3.9. I added test coverage but it seems like something in Pydantic doesn't get along with it right now. |
There was a problem hiding this comment.
Thanks for your work on this @kalzoo! I made a couple tweaks, feel free to take a look and let me know if you disagree with anything before I merge in.
Specifically:
- Identifiers more aggressively strip disallowed characters so that the ValueError isn't necessary
- The prefix is now configurable (as you recommended)
openapi_python_client/utils.py
Outdated
|
|
||
| if new_value.isidentifier(): | ||
| return new_value | ||
|
|
||
| raise ValueError(f"Cannot convert {value} to a valid python identifier") | ||
| return f"{FIELD_PREFIX}_{new_value}" |
There was a problem hiding this comment.
-
I'd suggest including the
_in the prefix itself (i.e.field_) rather than here, so the user can opt to omit it -
This could still be an invalid identifier - one case being that
FIELD_PREFIXitself has invalid characters. This could be checked here, inConfig, or not at all
There was a problem hiding this comment.
Good call, I've made the config change so people can omit the underscore if they want. For now I don't want to try and catch user-supplied errors, I'm pretty sure invalid classes can also be provided from the other config values in the current version.
This change allows the use of field names with leading characters (such as digits) which if converted directly into Python code would yield invalid identifiers.
Closes #203