Upgrade isort and enable black compat mode #248
Conversation
owais
requested review from
a team,
toumorokoshi and
ocelotl
and removed request for
a team
owais
force-pushed
the
isort-and-black-be-friends
branch
5 times, most recently
from
a833aa1
to
2fb39eb
Compare
|
pylint is crashing after the upgrade. I'll look into that and ping here when this is ready for review. |
owais
force-pushed
the
isort-and-black-be-friends
branch
6 times, most recently
from
16c685f
to
d9f1178
Compare
|
Why does this PR depend on [#231]? |
Might take some time given that the upgrade revealed tons of issues but will do, thanks. |
Doesn't hard depend on 231, just that there is duplicate work involved. Newer pylint reports exact issues as errors that 231 fixes so to make this PR pass, we either need to copy over changes from 231 or wait for it to merge and rebase. |
owais
force-pushed
the
isort-and-black-be-friends
branch
9 times, most recently
from
d3cbc30
to
97fa190
Compare
owais
force-pushed
the
isort-and-black-be-friends
branch
4 times, most recently
from
6c62cc2
to
afd8c5e
Compare
| @@ -23,6 +23,7 @@ __pycache__ | |||
| venv*/ | |||
| .venv*/ | |||
| opentelemetry-python-core*/ | |||
| /opentelemetry-python-core | |||
There was a problem hiding this comment.
What's the difference between this and above? Maybe they can be combined
There was a problem hiding this comment.
I used symlink instead of checking out another copy of core which did not work with the existing rule. Probably could be combined but I didn't understand fully how others checkout the repo so didn't want to break anyone's flow.
There was a problem hiding this comment.
might be better to move this to a .gitignore_global, if it's user-flow related.
But either not way not really worried about slightly bloating the gitignore. It may be good to add a comment to clarify this is for local dev scenarios.
| @@ -98,7 +98,7 @@ async def _(param: str): | |||
| return {"message": param} | |||
|
|
|||
| @app.get("/healthzz") | |||
| async def health(): | |||
| async def health(): # pylint: disable=unused-variable | |||
There was a problem hiding this comment.
How did this even trigger pylint. Seems like the new version is very noisy
There was a problem hiding this comment.
This is an unused var actually but there is no reason why it can't be named _ like the rest of the handlers defined inside this method. Updated.
| super-with-arguments, # temp-pylint-upgrade | ||
| isinstance-second-argument-not-valid-type, # temp-pylint-upgrade | ||
| raise-missing-from, # temp-pylint-upgrade | ||
| unused-argument, # temp-pylint-upgrade | ||
| protected-access, # temp-pylint-upgrade | ||
| super-init-not-called, # temp-pylint-upgrade | ||
| invalid-overridden-method, # temp-pylint-upgrade |
There was a problem hiding this comment.
what is is temp-pylint-upgrade?
There was a problem hiding this comment.
Just a comment to note down the rules we disabled when we upgraded pylint. Intention is to come back and fix these lint issues and remove the ignore rules.
| wrong-import-order, # Leave this up to isort | ||
| bad-continuation, # Leave this up to black | ||
| line-too-long, # Leave this up to black |
There was a problem hiding this comment.
Can these all be removed after upgrade? Looking at the black docs, the config for compatibility should be pretty minimal https://black.readthedocs.io/en/stable/compatible_configs.html#pylint
There was a problem hiding this comment.
I haven't looked into it but I'm for removing rules if we can automate the fixes with black+isort. I really really miss gofmt :(
owais
force-pushed
the
isort-and-black-be-friends
branch
from
afd8c5e
to
7dcc4a8
Compare
| @@ -23,6 +23,7 @@ __pycache__ | |||
| venv*/ | |||
| .venv*/ | |||
| opentelemetry-python-core*/ | |||
| /opentelemetry-python-core | |||
There was a problem hiding this comment.
might be better to move this to a .gitignore_global, if it's user-flow related.
But either not way not really worried about slightly bloating the gitignore. It may be good to add a comment to clarify this is for local dev scenarios.
owais
force-pushed
the
isort-and-black-be-friends
branch
6 times, most recently
from
d7ec26d
to
90b836f
Compare
|
Fixed new lint issues. We need to merge this soon as changes to master keeps breaking this PR. |
isort and black can be incompatible. Often isort re-writes files in a way black doesn't like. It takes quite some time and manual effort to make changes that satisfy both isort and black. Fortunately, newer versions of isort support a black compatibility mode by setting isort profile to "black". This makes isort order imports in a way that is compatible with how black formats Python code. This PR configures isort to use the black profile by default.
owais
force-pushed
the
isort-and-black-be-friends
branch
from
90b836f
to
ee0f81d
Compare
isort and black can be incompatible. Often isort re-writes files in a
way black doesn't like. It takes quite some time and manual effort to
make changes that satisfy both isort and black.
Fortunately, newer versions of isort support a black compatibility mode by setting
isort profile to "black". This makes isort order imports in a way that
is compatible with how black formats Python code. This PR configures
isort to use the black profile by default
Depends on #231