Latest 4.3.3 release changed how force_alphabetical_sort works incorrectly within a minor release and without proper documentation leading to breakage

[hvelarde]

I have a project running a code analysis that uses isort and it started failing with the new release as you can see here: https://travis-ci.org/simplesconsultoria/collective.texttospeech/jobs/118975159#L2414

the settings are inside the setup.cfg file.

downgrading to 4.2.2 solves the issues.

[timothycrosley]

Hi @hvelarde, the best I can tell the issue is actually that the imports where incorrectly sorted on isort 4.2.2 and that bug fixes in 4.2.3 have now caused the incorrect sorting to correctly be seen as an error. Is there a specific change that isort recommended that you disagree with based on your config? I checked out the project locally and I personally agreed with all its recommend changes.

Thanks!

~Timothy

[hvelarde]

I set from_first = True in my setup.cfg file and it is still failing; so I still suspect there is a bug in the new release.

do you mind to double check?

[hvelarde]

# bin/code-analysis
Check clean lines....................[ OK ] in 0.154s
Flake8..........................[ FAILURE ] in 1.136s
/home/hvelarde/collective/texttospeech/src/collective/texttospeech/tests/test_controlpanel.py:0:1: I001 isort found changes, run it on the file
/home/hvelarde/collective/texttospeech/src/collective/texttospeech/tests/test_resources.py:0:1: I001 isort found changes, run it on the file
/home/hvelarde/collective/texttospeech/src/collective/texttospeech/tests/test_robot.py:0:1: I001 isort found changes, run it on the file
/home/hvelarde/collective/texttospeech/src/collective/texttospeech/tests/test_setup.py:0:1: I001 isort found changes, run it on the file
/home/hvelarde/collective/texttospeech/src/collective/texttospeech/upgrades/v2/__init__.py:0:1: I001 isort found changes, run it on the file
The command "bin/code-analysis" exited with 1 in 1.172s.
# grep isort bin/code-analysis
    '/home/hvelarde/.buildout/eggs/flake8_isort-1.2-py2.7.egg',
    '/home/hvelarde/.buildout/eggs/isort-4.2.3-py2.7.egg',
# bin/isort src/collective/texttospeech/tests/test_controlpanel.py
# git diff
diff --git a/src/collective/texttospeech/tests/test_controlpanel.py b/src/collective/texttospeech/tests/test_controlpanel.py
index 3339c8f..cca8e7b 100644
--- a/src/collective/texttospeech/tests/test_controlpanel.py
+++ b/src/collective/texttospeech/tests/test_controlpanel.py
@@ -1,4 +1,6 @@
 # -*- coding: utf-8 -*-
+import unittest
+
 from collective.texttospeech.config import DEFAULT_ENABLED_CONTENT_TYPES
 from collective.texttospeech.config import PROJECTNAME
 from collective.texttospeech.interfaces import ITextToSpeechControlPanel
@@ -8,8 +10,6 @@ from plone.app.testing import logout
 from plone.registry.interfaces import IRegistry
 from zope.component import getUtility

-import unittest
-

 class ControlPanelTestCase(unittest.TestCase):

# cat setup.cfg
[isort]
force_alphabetical_sort = True
force_single_line = True
from_first = True
line_length = 200
lines_after_imports = 2
not_skip = __init__.py

[timothycrosley]

Hi @hvelarde, the issue is not the from_first, in the diff your isort is moving a stdlib import to the top in accordance with pep8 rules, did you actually intend to have stdlib libraries (unittest) below third / first party ones?

[hvelarde]

yes, in our style guide we don't differentiate among standard and third party libraries.

CC @gforcada @do3cc

[gforcada]

@timothycrosley the setting not being respected here would be the force_alphabetical_sort = True where one would expect all imports to be sorted alphabetically, regardless of any other option. That was the idea behind that configuration option.

[timothycrosley]

It looks like the commit that probably broke this is here: https://github.com/timothycrosley/isort/pull/395/files, I'll have to think more about this, for now it's probably safe to use 4.2.2. I feel there are multiple ways to interpret force_alphabetical_sort which is what's leading to the confusion. You can force it within sections, which seems logical for most cases, force_alphabetical, as it used to be implemented almost seems more aptly described force_single_section. Really, I think ideally in isort 5.0.0 there should be profiles built in for common coding styles (google, plone, pep8, isort recommended etc) that can be easily enabled as a single configuration option - as it feels force_alphabetical_sort was essentially a replacement for something of that nature.

Thanks!

~Timothy

[timothycrosley]

Also: I'll make sure to return the old behavior tonight in a 4.2.4 build, as in no case should such a major change to settings interpretation happen in a minor release, I apologize that this slipped through!

Thanks!

~Timothy

[gforcada]

@timothycrosley thanks for the review and the upcoming 4.2.4. Depending on such well maintained packages is great!

wrt. profiles, yes, that would be perfect for "our" usecase (plone styleguide).

[do3cc]

@timothycrosley Is there a roadmap towards this 5.0 with some ideas of included prefixes already?

[timothycrosley]

Hi @do3cc, I do not have a roadmap for this yet, but one thing I guarantee is that isort wont become purposely incompatible with the plone guideline. That's a priority for me. Using profiles, for common coding styles, and then writing tests for them makes it even more likely they wont break. Most likely once I sort 5.0.0 is out you'll just be able to do:

isort --plone

or:

[isort]
profile = plone

within your config.

Thanks!

~Timothy

[timothycrosley]

isort 4.2.4 has been released and should fix this issue, restoring the old behavior of force_alphabetical_sort. I've also added a baseline test to ensure isort continues to follow how I understand the Plone coding style for imports here: https://github.com/timothycrosley/isort/blob/develop/test_isort.py#L1771
Which will be expanded once profiles are put in place.

I apologize this issue occurred. Thanks everyone for helping to diagnose and your patience while it was resolved! Please let me know if you run into any further issues.

Thanks!

~Timothy

[hvelarde]

tested and working, thank you, very much, @timothycrosley!