Skip to content
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

feat: new cli option --use-exact-imports #1983

Merged
merged 7 commits into from
Jun 18, 2024

Conversation

alpoi-x
Copy link
Contributor

@alpoi-x alpoi-x commented Jun 2, 2024

Adds a new CLI options --use-exact-imports (off by default) to import types directly instead of modules. For example:

from . import bean_type

class Bean(BaseModel):
    bean_type: bean_type.BeanType

becomes

from .bean_type import BeanType

class Bean(BaseModel):
    bean_type: BeanType

Resolves #1982 and potentially #1683

@lord-haffi
Copy link
Contributor

lord-haffi commented Jun 3, 2024

This would indeed fix #1683. Also supersedes #1684.

Copy link

codecov bot commented Jun 3, 2024

Codecov Report

Attention: Patch coverage is 84.61538% with 2 lines in your changes missing coverage. Please review.

Please upload report for BASE (main@f023016). Learn more about missing BASE report.

Files Patch % Lines
datamodel_code_generator/parser/base.py 77.77% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@           Coverage Diff           @@
##             main    #1983   +/-   ##
=======================================
  Coverage        ?   98.81%           
=======================================
  Files           ?       37           
  Lines           ?     4212           
  Branches        ?      981           
=======================================
  Hits            ?     4162           
  Misses          ?       31           
  Partials        ?       19           
Flag Coverage Δ
unittests 98.48% <84.61%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link

codspeed-hq bot commented Jun 3, 2024

CodSpeed Performance Report

Merging #1983 will not alter performance

Comparing alpoi-x:use-exact-imports (b6810b5) with main (f023016)

Summary

✅ 29 untouched benchmarks

Copy link
Contributor

@lord-haffi lord-haffi left a comment

Choose a reason for hiding this comment

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

lgtm 👍
Feel free to also link #1683 to this PR, I can't do it myself.

Comment on lines +713 to +716
if imports.use_exact:
from_, import_ = exact_import(
from_, import_, data_type.reference.short_name
)
Copy link
Owner

Choose a reason for hiding this comment

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

The repo prefer cover almost lines by the unittest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hiya @koxudaxi, I'm not entirely sure how to test these lines 😓 I can't find any existing tests for this class method (Parser.__change_from_import) so I've got nothing to base it on.

Copy link
Contributor

@lord-haffi lord-haffi Jun 17, 2024

Choose a reason for hiding this comment

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

@alpoi-x I think there are tests for executing the whole command. You could just take one of these and test the new option with multi-file-output (I don't remember where this test is but I think, I saw it few months ago). You could then assert that the imports look the way you want.

Copy link
Owner

@koxudaxi koxudaxi left a comment

Choose a reason for hiding this comment

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

@alpoi-x @lord-haffi
Thank you for creating the PR.
I left a few comments.
Otherwise, It looks good!!

@koxudaxi koxudaxi merged commit 28be37d into koxudaxi:main Jun 18, 2024
92 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

AttributeError: 'FieldInfo' object has no attribute '<EnumName>'
3 participants