-
-
Notifications
You must be signed in to change notification settings - Fork 313
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
Add formatter module #1746
base: main
Are you sure you want to change the base?
Add formatter module #1746
Conversation
CodSpeed Performance ReportMerging #1746 will not alter performanceComparing Summary
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1746 +/- ##
===========================================
- Coverage 100.00% 99.51% -0.49%
===========================================
Files 38 43 +5
Lines 4212 4338 +126
Branches 973 992 +19
===========================================
+ Hits 4212 4317 +105
- Misses 0 15 +15
- Partials 0 6 +6
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Log from single failed test: ChefBuildError
Backend 'setuptools.build_meta:__legacy__' is not available.
at ~\.local\venv\Lib\site-packages\poetry\installation\chef.py:152 in _prepare
148|
149| error = ChefBuildError("\n\n".join(message_parts))
150|
151| if error is not None:
> 152| raise error from None
153|
154| return path
155|
156| def _prepare_sdist(self, archive: Path, destination: Path | None = None) -> Path:
Note: This error originates from the build backend, and is likely not a problem with poetry but with lazy-object-proxy (1.9.0) not supporting PEP 517 builds. You can verify this by running 'pip wheel --use-pep517 "lazy-object-proxy (==1.9.0)"'. |
@denisart could you please rebase the branch? |
@denisart the best is that you add the write permission of the PR to me |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denisart
Thank you for the great PR.
I left a few comments.
import_ = Import.from_full_path(custom_formatter_import) | ||
imported_module_ = import_module(import_.from_) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't think the uses case 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also had the same reaction. But the Import
class is very suitable in this case.
if default_formatters is None: | ||
return self._load_formatters(self._default_formatters) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does the method expect the path string?
I prefer custom_formatter: BaseCodeFormatter
to raw str.
We can use type-checking.
If we want to pass the custom formatted class from CLI, how about loading the custom formatted class from str only for that?
I feel that passing raw str
is a bit confusing, since it is also intended to be used as a module by the user.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems I didn't fully understand your proposal. Can you explain or give an example?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@denisart
I'm sorry I show you the code.
I expect the arguments.
class CodeFormattersRunner:
...
def __init__(
self,
disable_default_formatter: bool = False,
default_formatter: Optional[List[BaseCodeFormatter]] = None,
custom_formatters: Optional[List[BaseCodeFormatter]] = None,
if we give the external formatter to CodeFormattersRunner
, we should get the BaseCodeFormatter
by using
load_code_formatter
custom_comatter = load_code_formatter("my_package.my_sub_package.FormatterName")
runner = CodeFormattersRunner(custom_formatters=[custom_comatter])
I don't think CodeFormattersRunner
must know the full path string; CodeFormattersRunner
only needs to handle the CodeFormatter class. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@koxudaxi ,
I understood your idea. Do you propose to move the loading of formatters from CodeFormattersRunner
? This is good from an architectural point of view.
Do you propose loading formatters by path in generate
method from datamodel_code_generator/__init__.py
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understood your idea. Do you propose to move the loading of formatters from CodeFormattersRunner? This is good from an architectural point of view.
Yes!!
Do you propose loading formatters by path in generate method from datamodel_code_generator/init.py?
Yes, it is. Thank you for saying what I was trying to say.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I agree with you. I'll create a fix.
Example:
Added isort, black and ruff (empty now) formatters based on
datamodel_code_generator.formatter.base.BaseCodeFormatter
.New formatters runner (based on
CodeFormatter
fromdatamodel_code_generator.format
). Usage example: