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

Add formatter module #1746

Open
wants to merge 10 commits into
base: main
Choose a base branch
from
Open

Conversation

denisart
Copy link
Contributor

@denisart denisart commented Nov 27, 2023

  1. Added a new formatter module (now is not using in main code).

Example:

from datamodel_code_generator.formatter.base import BaseCodeFormatter

class CustomHeaderCodeFormatter(BaseCodeFormatter):
        formatter_name: ClassVar[str] = "custom"

        def __init__(self, formatter_kwargs: Dict[str, Any]) -> None:
            super().__init__(formatter_kwargs=formatter_kwargs)
            default_header = "my header"
            self.header: str = self.formatter_kwargs.get("header", default_header)

        def apply(self, code: str) -> str:
            return f'# {self.header}\\n{code}'
        
formatter_kwargs = {"header": "formatted with CustomHeaderCodeFormatter"}
formatter = CustomHeaderCodeFormatter(formatter_kwargs)

code = '''x = 1\ny = 2'''
print(formatter.apply(code))
"""
# formatted with CustomHeaderCodeFormatter
x = 1
y = 2
"""
  1. Added isort, black and ruff (empty now) formatters based on datamodel_code_generator.formatter.base.BaseCodeFormatter.

  2. New formatters runner (based on CodeFormatter from datamodel_code_generator.format). Usage example:

runner = CodeFormattersRunner(
    default_formatter=['black', 'isort'],
    disable_default_formatter=False,
    custom_formatters=["my_package.my_sub_package.CustomHeaderCodeFormatter"],
    custom_formatters_kwargs={
        'custom': {
            'header': 'formatted with CustomHeaderCodeFormatter',
        }
    }
)

runner.format_code("...")
# running black
# running isort
# running custom

Copy link

codspeed-hq bot commented Nov 27, 2023

CodSpeed Performance Report

Merging #1746 will not alter performance

Comparing denisart:add-formatter-module (c33815c) with main (f856d13)

Summary

✅ 31 untouched benchmarks

Copy link

codecov bot commented Nov 27, 2023

Codecov Report

Attention: Patch coverage is 83.33333% with 21 lines in your changes missing coverage. Please review.

Project coverage is 99.51%. Comparing base (f856d13) to head (c33815c).

Files Patch % Lines
datamodel_code_generator/formatter/base.py 87.93% 6 Missing and 1 partial ⚠️
datamodel_code_generator/formatter/black.py 78.78% 4 Missing and 3 partials ⚠️
datamodel_code_generator/formatter/isort.py 73.91% 4 Missing and 2 partials ⚠️
datamodel_code_generator/formatter/ruff.py 87.50% 1 Missing ⚠️
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     
Flag Coverage Δ
unittests 99.14% <81.74%> (-0.53%) ⬇️

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.

@denisart denisart marked this pull request as draft November 27, 2023 15:33
@denisart denisart marked this pull request as ready for review November 27, 2023 20:25
@denisart
Copy link
Contributor Author

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)"'.

@koxudaxi
Copy link
Owner

koxudaxi commented Dec 2, 2023

@denisart could you please rebase the branch?
I just updated the poetry version and merged it to main.

@koxudaxi
Copy link
Owner

koxudaxi commented Dec 2, 2023

@denisart the best is that you add the write permission of the PR to me

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.

@denisart
Thank you for the great PR.
I left a few comments.

Comment on lines +72 to +73
import_ = Import.from_full_path(custom_formatter_import)
imported_module_ = import_module(import_.from_)
Copy link
Owner

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 😅

Copy link
Contributor Author

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.

Comment on lines +152 to +153
if default_formatters is None:
return self._load_formatters(self._default_formatters)
Copy link
Owner

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.

Copy link
Contributor Author

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?

Copy link
Owner

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?

Copy link
Contributor Author

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?

Copy link
Owner

Choose a reason for hiding this comment

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

@denisart

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.

Copy link
Contributor Author

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.

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.

2 participants