Migrate isort #29
Migrate isort #29
Conversation
There was a problem hiding this comment.
Cool, thanks! It's awesome that this fixes some other TODOs too.
I think we want to setup an .isort.cfg for Slicker itself. It can say to put only one import per line, which will avoid a bunch of the non-whitespace test changes (and match Khan style). And it can say to treat any module starting with third_party as third party, which will fix the skipped test. I'm in fact already working on writing such a file which we'll be using in all Khan repos; if you want to just wait on that I'll hopefully have something workable this week. Or if you want to go ahead, go for it.
| @@ -1,8 +1,5 @@ | |||
| from __future__ import absolute_import | |||
|
|
|||
| # TODO(benkraft): In the case where these two imports are rewritten to be | |||
There was a problem hiding this comment.
Oh nice! Didn't realize isort would solve this for us.
| @@ -174,6 +174,8 @@ def test_moving_implicit(self): | |||
| 'moving_implicit', | |||
| 'foo.secrets.lulz', 'quux.new_name') | |||
|
|
|||
| @unittest.skip("TO DISCUSS fix_python_imports is now moved below " | |||
There was a problem hiding this comment.
Heh, this one it's probably easiest to just change the import to be from third_party import fix_python_imports, in which case the .isort.cfg I mention will suffice.
There was a problem hiding this comment.
When I try with the following:
[settings]
known_third_party=fix_python_imports
It is different:
+ import fix_python_imports
+
import codemod_fork as the_other_codemodThere was a problem hiding this comment.
Oh, I see. This is a real issue with isort IMO -- see PyCQA/isort#468 -- but we can just work around it for now -- simplest is probably to just remove the sys.path line from the test data, and resort both input and output to do what isort wants.
Though this makes me wonder: until that issue is fixed, is it better to simply skip isort for any file that has code before imports? That would be a little sad -- it's a slight regression from fix-includes I think -- but maybe better than making a mess of it. Maybe @csilvers has an opinion on this question.
There was a problem hiding this comment.
I definitely think we don't want to let isort sort imports around code -- that will break things, like in the example at PyCQA/isort#693. Trying to work around the problem is easy for tests but not for actual real code.
I think what we'd have to do is to change slicker to notice blocks of imports without any other code, call isort on those blocks individually, and then merge them all together. I don't even know if isort will work that way, or if it needs to know the entire file contents.
It may just be easiest to hold off on porting to isort until this is fixed. Sad though.
| @@ -820,7 +827,6 @@ def test_uses_old_module_imports_self_via_relative_import(self): | |||
| project_root=self.tmpdir) | |||
| self.assertFileIs('foo.py', | |||
| ('from __future__ import absolute_import\n\n' | |||
| '\n\n' # TODO(benkraft): remove extra newlines | |||
There was a problem hiding this comment.
Another nice side benefit!
|
|
||
| from fix_includes import fix_python_imports | ||
| from isort import SortImports |
There was a problem hiding this comment.
This should be import isort, then isort.SortImports below, to conform to Khan's python style.
Fix #24
First of all, I must say your code is well commented, it helps a lot 👍
I tried my best to migrate to isort (and going one step closer to python 3 compatibility 😁). But I had to left one commit to discuss with you. I added the reason in the skip, and I hope this make sense.
Fell free to ask for details if needed.