upgrade to isort5 #4399
upgrade to isort5 #4399
Conversation
|
restyled-isort does not have isort5 support. will try adding later. |
| known_first_party=dvc,tests | ||
| known_third_party=PyInstaller,RangeHTTPServer,boto3,colorama,configobj,distro,dpath,flaky,flufl,funcy,git,grandalf,mock,moto,nanotime,networkx,packaging,pathspec,pygtrie,pylint,pytest,requests,ruamel,setuptools,shortuuid,shtab,toml,tqdm,voluptuous,yaml,zc | ||
| line_length=79 |
There was a problem hiding this comment.
Personally, I think 79 is fine. I'm not opposed to using 88, but I would prefer we don't go much higher than that in the future.
IMO Python projects that use 100+ tend to get hard to read, but I get that this is more of a personal taste issue than anything else.
There was a problem hiding this comment.
I have no strong opinion on that, only thing that bugs me is that this decision will require some refactor, as black tends to not remove " when its not needed anymore, so we will probably have a lot of lines like
"This is " "the comment"
There was a problem hiding this comment.
@pared, string handling has been significantly improved in black (psf/black#1132) and should solve that issue. However, that'd be useful only if there were a new release. :(
There was a problem hiding this comment.
Okay then, if no one has strong opinion, I'll be increasing the limit to 88.
CC @efiop
There was a problem hiding this comment.
Oops, missed this one. I would not increase it to 88. 80 is a good standard width that is used in many projects and is on the border of being viewable in splitscreens on github and/or editors (e.g. vim). Plus, if we change it now, we'll introduce inconsistency with the rest of the code base. So I'm not really seeing good reasons to go to 88, it won't make it easier to navigate.
|
failing CI test is specific to experiments and shouldn't block this PR (see #4401) |
I'll rerun later. Thanks. Currently it's blocked by lack of support from restyled. Keeping this as draft, till then, see restyled-io/restylers#86. |
|
PR has been merged and the new |
* Imports are no longer moved to top. * `seed-isort-config` is no longer needed. * Has a `black`-compatible profile/config. * Also `isorts` the imports anywhere in the code (not just at the top). Also bumps `restyled-isort` to newest version
Imports are no longer moved to the top.
seed-isort-configis no longer needed.Has a
black-compatible profile/config.Also
isorts the imports anywhere in the code(not just at the top).
❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. 🙏