python: several typing and style improvements#16378
python: several typing and style improvements#16378wing328 merged 4 commits intoOpenAPITools:masterfrom
Conversation
There was a problem hiding this comment.
Use of (object) as a base class has not been recommended for a long time.
jessemyers-lettuce
force-pushed
the
typing-and-style-improvements
branch
from
d16c6d7 to
d6af4ce
Compare
There was a problem hiding this comment.
The return type of -> None is necessary for the type checker to see a newly constructed instances as having a type. Otherwise, we end up with everything being any-typed.
There was a problem hiding this comment.
This alignment avoids line-length issues (using black's line length configuration).
There was a problem hiding this comment.
Some formatters (e.g. black) will change:
raise ValueError("long string") # noqa: E501to:
raise ValueError(
"long string"
) # noqa: E501thereby moving the noqa comment to the wrong location; declaring the message on its own line avoids this problem.
There was a problem hiding this comment.
flake8 does not like trailing spaces
There was a problem hiding this comment.
This was just bad Python (and flake8 notices this)
jessemyers-lettuce
force-pushed
the
typing-and-style-improvements
branch
from
d6af4ce to
77534ca
Compare
The generated (python) code fails with several standard validation tools, including `flake8`, `mypy`, and `autoflake`. While fixing every possible violation -- especially wrto typing -- woudl be a project, some of the changes are fairly easy. - The `autoflake` tool picks up on unused imports. These can just be removed. - The `mypy` tool picks up on numerious typing violations, especially if set to its strictest mode. As a starting point, all functions ought to annotate a return type, including constructors, even if the return type is `None` because otherwise the functions are omitted from type checking and it's impossible to make incremental progress towards adding types. - The `flake8` tool mostly finds whitespace and line-length issues; while line-length standards very, the source already includes several flake8 ignores, so it seems safe to add a few more.
jessemyers-lettuce
force-pushed
the
typing-and-style-improvements
branch
from
77534ca to
6e25048
Compare
| import {{packageName}} | ||
| from {{apiPackage}}.{{classFilename}} import {{classname}} # noqa: E501 | ||
| from {{packageName}}.rest import ApiException |
There was a problem hiding this comment.
It's not clear what the right behavior is for a test case that is meant to be edited by hand. On the one hand, adding imports for likely usages is helpful; on the other hand, it's not great to generate code that doesn't pass the linter because it includes unnecessary imports.
There was a problem hiding this comment.
What about commenting out these imports instead so that users can easily uncomment these imports if needed?
| def setUp(self): | ||
| self.api = {{apiPackage}}.{{classFilename}}.{{classname}}() # noqa: E501 | ||
| def setUp(self) -> None: | ||
| self.api = {{classname}}() # noqa: E501 |
There was a problem hiding this comment.
The import block was importing both the package and the class; we only need one of these. It's more common in Python to use from foo import Bar style so that Bar() can be called without qualification.
There was a problem hiding this comment.
+1, and I recently did the same in #16375 for python-flask.
Maybe we should start an issue that lists all the syntax and formatting issues of python projects, most likely, they will apply to most of them
|
@jessemyers-lettuce thanks for the PR.
@anis-campos yes please. Always good to have a ticket (issue) for tracking. |
|
@jessemyers-lettuce can you please follow step 3 in the PR checklist to update the samples so that the CI can verify the change? |
wing328
added
Client: Python
Enhancement: Code format
Enhancement: Code Cleanup
|
@wing328 I wanted to, but I don't have a working JVM in my setup; I invoke the generator via docker and work in other languages. If there's a way to run the build scripts through |
|
@wing328 I was able to get OpenJDK working and have now submitted. There is an error that I do not understand. The removal of the unused I'm not sure what to make of this; clearly some encapsulation boundary is being violated, but I'm reluctant to put in a great deal of effort if the scope of making improvements here requires developing deeper knowledge of the project; I'd hoped just to be able to improve the templates and move on. Please advise. |
|
FYI @taxpon (2017/07) @frol (2017/07) @mbohlool (2017/07) @cbornet (2017/09) @kenjones-cisco (2017/11) @tomplus (2018/10) @arun-nalla (2019/11) @krjakbrjak (2023/02) |
jessemyers-lettuce
force-pushed
the
typing-and-style-improvements
branch
from
e604a57 to
29e4f89
Compare
|
can you please update the samples again ? |
|
@wing328, I did, but I'm not seeing any changes when I do. |
|
python tests with updated samples passed via #16477. I'll merge this one and update the samples after. Thanks again for the PR |
3 participants
The generated (python) code fails with several standard validation tools, including
flake8,mypy, andautoflake. While fixing every possible violation -- especially wrto typing -- woudl be a project, some of the changes are fairly easy.The
autoflaketool picks up on unused imports. These can just be removed.The
mypytool picks up on numerious typing violations, especially if set to its strictest mode. As a starting point, all functions ought to annotate a return type, including constructors, even if the return type isNonebecause otherwise the functions are omitted from type checking and it's impossible to make incremental progress towards adding types.The
flake8tool mostly finds whitespace and line-length issues; while line-length standards very, the source already includes several flake8 ignores, so it seems safe to add a few more.