tidy-import unintentional changes post v1.8.6 (PyInf#11818)

[dshivashankar1994]

The below script is clean and tidy-imports (before 1.8.6) doesn't complain anything

> cat /var/tmp/a.py
import os
import simplejson
import sys

from   .constants                import abc
from   dshiva.util               import DictObject
from   dshiva.aaa.exception      import CustomURLValidationException
from   dshiva.test.test.test.util \
                                 import (get_modules,
                                        get_functions_test)
from   dshiva.test.log           import error, warning
from   dshiva.python.test        import resolve
from   dshiva.python.new.test    import hostname, username

def test_function():
    from   jsonschema               import validate
    pass

But the latest version is suggesting some improper changes

> tidy-imports /var/tmp/a.py --no-remove-unused
--- /var/tmp/a.py   2024-01-23 13:17:38.069534798 -0500
+++ /tmp/tmp6qsaavvn    2024-01-23 13:17:52.401467605 -0500
@@ -1,8 +1,6 @@
 import os
-import simplejson
 import sys

-from   .constants               import abc
 from   dshiva.aaa.exception     import CustomURLValidationException
 from   dshiva.python.new.test   import hostname, username
 from   dshiva.python.test       import resolve
@@ -11,8 +9,11 @@
                                 import get_functions_test, get_modules
 from   dshiva.util              import DictObject

+from   .constants               import abc
+import simplejson
+
 def test_function():
-    from   jsonschema               import validate
+    from jsonschema import validate
     pass

Replace /var/tmp/a.py? [y/N] 

The expected suggestion after #13 should be

from   .constants               import abc

from   dshiva.aaa.exception     import CustomURLValidationException
from   dshiva.python.new.test   import hostname, username
from   dshiva.python.test       import resolve
from   dshiva.test.log          import error, warning
from   dshiva.test.test.test.util \
                                import get_functions_test, get_modules
from   dshiva.util              import DictObject

import os
import simplejson
import sys

Where similar package items are grouped and still sorted lexicographically.

[dshivashankar1994]

@Carreau Can you provide an ETA here ?

[Carreau]

I'm back from Time Off starting today, I will have a look.

[Carreau]

You will have to be more precise about which changes are improper and what you expect. The new ordering past 1.8.6 was an explicit changes requested in issue #13.

I think that the new logic separate builtins modules at the top, so I expect simplejson to move. I guess .constant is weird, and was likely not though of, we can special case it, but it fall into the "imports that appear only once so should not be treated specially" and appear in lexicographical order as the first.

As pyflyby can't know dshiva is a special package it treats it as any other an external package (numpy/scipy) and sort it lexicographically.

If you do care about the jsonschema import in the function I can look into it more.

But my impression from #13 is that pyflyby should rely more on tools like isort and get closer to other standard than to implement custom logics ? But if you can better describe the logic you wish we can do out best to implement it.

[dshivashankar1994]

The statements from .constants import abc and from jsonschema import validate are the imports I want to bring to notice.

I believe if there are more imports with "from ." (like "from .foo import abc; from .bar improt def"), they need to be grouped together.

I see that the ask in #13 is to sort the imports lexicographically. So the group 1 (dshiva) should occur at the start and then the rest was my though

The expected should have been

from   .constants               import abc

from   dshiva.aaa.exception     import CustomURLValidationException
from   dshiva.python.new.test   import hostname, username
from   dshiva.python.test       import resolve
from   dshiva.test.log          import error, warning
from   dshiva.test.test.test.util \
                                import get_functions_test, get_modules
from   dshiva.util              import DictObject

import os
import simplejson
import sys

[dshivashankar1994]

Also noting that in case like below, new lines introduced before datiby.logging is not correct

> cat /var/tmp/c.py
import os
import sys
from   datiby.cdefghtil         import DictObject
from   datiby.defghi.exception  import CustomURLValidationException
from   datiby.defghi.service_page.util \
                                import (get_modules,
                                        get_slong_name_very_very_long)
from   datiby.logging           import error, warning
from   datiby.qzuipm.imp        import resolve
from   datiby.qzuipm.os         import hostname, username
from   .constants               import DIRdefghi_instance
import simplejson

def test_function():
    from jsonschema import validate
    pass

Tidy-imports run:

> tidy-imports /var/tmp/c.py --no-remove-unused
--- /var/tmp/c.py   2024-01-31 02:13:32.522248105 -0500
+++ /tmp/tmpp7knlkl_    2024-01-31 02:13:40.166268972 -0500
@@ -1,13 +1,16 @@
 import os
 import sys
+
 from   datiby.cdefghtil         import DictObject
 from   datiby.defghi.exception  import CustomURLValidationException
 from   datiby.defghi.service_page.util \
                                 import (get_modules,
                                         get_slong_name_very_very_long)
+
 from   datiby.logging           import error, warning
 from   datiby.qzuipm.imp        import resolve
 from   datiby.qzuipm.os         import hostname, username
+
 from   .constants               import DIRdefghi_instance
 import simplejson

Replace /var/tmp/c.py? [y/N] y

[sac111gp]

This is a blocker for us to release 1.8.8 which has fixes for many other issues and are pending deployment. @Carreau could you check this? Let us know if something is not clear yet.

cc @quarl

[Carreau]

I will see what I can do, but there are a few limitation:

1) .constant is going to be hard to fix as it requires inferring the name of the current module from the filename, which maybe invasive changes to the API. Without that we can't know which group .constant refers to.

2) #13 was fixed by #263, which uses isort, I believe it will be hard to customize isort to sort exactly the way you like, so we might have to write our custom import sorter.

I see what I can do, but maybe in the short term we should just revert #263.