-
-
Notifications
You must be signed in to change notification settings - Fork 315
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.
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
Unused, but requied target_datetime_class
argument in get_data_model_types()
#2195
Comments
thorwhalen
added a commit
to thorwhalen/datamodel-code-generator
that referenced
this issue
Nov 27, 2024
thorwhalen
added a commit
to thorwhalen/datamodel-code-generator
that referenced
this issue
Nov 27, 2024
koxudaxi
added a commit
that referenced
this issue
Dec 14, 2024
* fix: get_data_model_types argument target_datetime_class has None default. See #2195 * fix: target_datetime_class defaults to DatetimeClassType.Datetime. See #2195 * fix: set defaults in module to make it easier to move to global defaults * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Koudai Aono <[email protected]>
I think It's resolved |
Good. Thanks. Enjoy. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Describe the bug
It's not a bug, but a code smell that led to errors on my side.
Simply put:
target_datetime_class
is not used in the get_data_model_types function, and yet it's a required argument.To Reproduce
Expected behavior
I'd expect it to not fail, since the third,
target_datetime_class
, is not used anyway.I'd suggest setting a default:
target_datetime_class: Optional[DatetimeClassType] = None
, since it's not used anyway. See this fork branchtarget_datetime_class: DatetimeClassType = DatetimeClassType.Datetime
, if we want an actual useful choice. See this fork branchIn fact -- personally, I'd set a default for the
target_python_version
as well, setting it to the python version where the module is being run.Yes, explicit over implicit, and having defaults has disadvantages, always. It does a lot for UX though.
Personally, I'd set these defaults in the
format.py
module, where theDataModelType
andPythonVersion
enums are defined, and use them throughout.I tried, but ran into some test errors I didn't understand, so defined the defaults in
model/__init__.py
itself in this pull request #2196.Version:
The text was updated successfully, but these errors were encountered: