ISort does use isort default and not plone.api conventions

[do3cc]

:-(

[gforcada]

You need to provide an .isort.cfg configuration.

Copy it from p.r.codeanalysis: https://github.com/plone/plone.recipe.codeanalysis/blob/master/.isort.cfg

[do3cc]

Where is this documented? why is this not the default?

[gforcada]

Because this is coming from flake8-isort from p.r.codeanalysis point of view is just something that runs while running flake8

[gforcada]

What can be done is to add arguments on flake8-isort so that we can force them to plone styleguide on flake8-isort while keeping it generic for anyone to use.

And that's why I closed the issue before, the problem is on flake8-isort not on p.r.codeanalysis. I let you close this time :)

[do3cc]

plone.recipe.codeanalysis gives unreliable and wrong results because it uses flake8-isort. It breaks CI environments right now, maybe it should just be blacklisted, or maybe it doesn't matter which repo needs the error report because its both maintained by the same people.

[gforcada]

You are totally free to enable or disable plugins as you like, if you add .isort.cfg on your repos, or automatically on CI before running code analysis you will be fine.

[saily]

@gforcada you dropped a well reviewed code and replaced it with a fast-track released flake8-isort. Does flake8-isort support to inject some default values, because i really don't want to add a .isort.cfg to each package in my vcs.

[gforcada]

The idea of creating flake8 plugins is that apart from flake8-plone-api and flake8-plone-hasattr they are generic to be reused by any other python project, and as in this plugin there is already a way to configure it, problem is solved.

As I said, we can add arguments to it and passing them via p.r.codeanalysis, but right now you have a solution at your fingertips.

[gforcada]

@saily flake8-isort is just a shim over isort, and that's not a fast-tracked project. Just disable it if it doesn't suite your needs, or provide pull requests to improve it.

You can even stick to an older release of p.r.codeanalysis until you are fine with the last one.

That's why it was changed from 2.0.X to 2.1. semver at its best.

[saily]

i'm pretty sure all of us experienced ploners know how to handle this, but the point is you're making it pretty complex for new developers because behaviour changed completely after a minor release upgrade from 2.0 to 2.1. And the worst thing about it is, 2.1 is not finished at all and people have to disable behavior to get valid code pass the validation. sorry, but that's a brown-bag release for me!

[do3cc]

I agree.
There is no disagreement that using flake plugins are a better way to handle stuff.
Releasing half working stuff, breaking others people code and then saying, fix it, is not a good behaviour.

Even with the undocumented .isort.cfg which is just lying around in this repo, a half decent project would have created broken code. long import lines were broken into multiple lines and sorted in an order that was not valid python code any more. I have no idea how this could end up in a release. (I fixed it in here in the meantime)

[tisto]

@saily @do3cc since I made the release I guess I'm the one to blame here. I was under the assumption that the flake8-plugins would be a simple drop-in replacement. I did a quick test before the release. Since I haven't used the sort feature yet, I did not catch that problem. Though, we are taking about one regression here, as far as I can see. Calling 2.1 a brown bag release is not really encouraging people to fix the problem. We should have made a 3.0 release and document that breaking change in a better way, no question about that.

[saily]

We're not blaming people here and we're not requesting a fix, we just don't want to close the issue as far as i see!

[tisto]

@saily that's fine with me. :)

[do3cc]

@saily I think, that the .isort.cfg actually makes sense. Because then you have the same config for each team member, not only for the checker, but for the editor integration to automatically sort imports.
Maybe it should be auto generated, but I am not sure about that

[gforcada]

Should we close this already? Maybe some docs are needed only?

[gforcada]

Since 25 days nobody objected, so closing now.

[do3cc]

But has it been fixed?

[saily]

I really hate the way we just ignore crap. Introducing a new import checker which checks for the wrong conventions by default is crap. So leave this open until it's fixed.

[tisto]

@saily gil asked 26 days ago if we can close the issue. If nobody objects it's perfectly ok to close the issue. Calling things "crap" does not really help either to motivate people to put effort into fixing it...I'm ok with keeping this issue open if the new checker breaks with the old convention.

[saily]

@tisto i disagree here. i invested a lot of time implementing the original sort checker. The community was inconsistent about their own conventions and @gforcada came up with the discussion changing the conventions. After that he claimed about my wrong implementation and implemented his own one, which checks wrong by default as well.

I do not understand how we as testing team can accept replacements which do not work in their shipped configuration. This doesn't help at all and just makes it even more hard to start writing valid code for beginners.

Sorry for being upset, but from my point of view this is really "crap".

[do3cc]

I just asked a question if it is fixed. If it isn't the ticket can be closed if we revert the commits that break out of the box sort behavior.
If it is fixed, the ticket can stay closed, of course

[tisto]

@saily @gforcada @do3cc Ok, I haven't had much time lately to look into that. I just somehow tried to follow the discussion. What about scheduling a quick hangout or IRC meeting to discuss things directly? I'm sure we can quickly resolve the issue that way...

[gforcada]

@tisto I was trying to write something polite but I fail at it, so don't expect me on that meeting, I just can not see how we can discuss if things are ruled out directly.

I see a lot of options before insulting others works:

• fix isort to do the sorting as one pleases
• improve flake8-isort to hardcode our holy-grail guidelines
• create a new flake8 plugin that brings back the old sorting method
• document it even more, if that's actually needed

but just providing stop energy and insulting is not my way of work sorry.

@do3cc as you already did before, fixing isort upstream is the way to go IMHO, my changes and your changes are already merged and released, so chances are that if you provide the improvements they will be accepted and released.

[do3cc]

Let us bring the discussion back on track.
This is not about @saily and myself being unhappy about the new default sort rule of case insensitiveness.
This is just about the question if currently plone.recipe.codeanalysis now sorts correctly according to the current rules (case insensitive) or still after the isort default. Because if it still ignores the plone.api conventions, it is broken.

[jensens]

Folks, mistakes or even not mistakes but decisions not all of us are happy with just happen. Keep calm and fix it :-)

Explicit is better than implicit

What about adding an option to the flake8 plugin, that requires a existing file .isort.cfg? Like flake8-isort-enforce-configuration and set it by default to true. Because implicit sorting rules are bad sorting rules.
If there is no .isort.cfg is there the check fails.

Then we add to all plone core packages the .isort.cfg configured to follow plone-api conventions (I suppose this can be automated, @mauritsvanrees did this with the CONTRIBUTING.rst some weeks ago).

[do3cc]

I'd be fine with an option. I created the Pull Request yesterday because I was bitten by it again. A project that uses plone.app.codeanalysis, lots of false errors because no .isort file.

[gforcada]

Just before leaving for Christmas vacations: flake8-isort 1.0

Without any additional configuration it will show:
I002 no .isort.cfg file found

if no .isort.cfg file is found.

Adding a --no-isort-config will bypass the check for .isort.cfg.

[do3cc]

@gforcada I am sorry, but after having worked with the isort plugin for a while, I am now against adding this to plone.recipe.codeanalysis at all. This is also in part a tooling problem that in theory could be fixed.

I am against adding the flake8-isort plugin, because it is a lot of work to get it right for every developer, for a very small benefit.

Here are the problems:

• They way .isort.cfg is looked up, forces me to put .isort.cfg into multiple places right now. Location of .isort.cfg gforcada/flake8-isort#2 This could probably be fixed in isort
• The documentation of what each meaning of .isort.cfg means is severely lacking. I only was able to create a working file after reading the source code. From looking at .isort.cfg a developer can not guess what the right way of sorting is. You force the user to install isort to let isort handle the sorting.
  The documentation could be fixed but the complexity because of the many things isort provides cannot be hidden.
• I am a vim user, there is a plugin for isort in vim. It sometimes randomly fails. I removed it from my plugins again, but now that you provided a fix flake8-isort, I really wanted to give all of this another try, activated the plugin again and again it did not work this time. I debugged it to find out that vim has chosen to use the python from a completely different virtualenv than the one I was working on at that moment. I do not know if this information is cached or queried from other still running vim sessions. I had to go to the other virtualenv and install isort there, to get the isort plugin working this time. Maybe on my next reboot the issue repeats, because vim chose another virtualenv again.

Because of this, I'd prefer to see the old sort plugin back which, also seems to conform to the plone.api style guide soonishly again. plone/plone.api#187 (comment)

I know that this is a tough because a lot of emotions and energy have been put into all of this. I hope you see that I have given the isort option a serious chance and used it extensively. I even managed to get it working for me. I just don't see that all this work I had is justified for every one who might be willing to contribute to a project that uses plone.app.codeanalysis[recommended]

[gforcada]

@do3cc, in reverse order:

• your third point is, sorry not an issue, what about emac users, what about pycharm users? If your editor does not work is not the tool problem but your integration. (related: https://github.com/timothycrosley/isort/wiki/isort-Plugins)
• lack of documentation? Have you seen isort wiki?
• location of .isort.cfg, again something that should be fixed on isort itself. Actually while adding the changes for that I wanted to reuse isort logic to find it, but I did not find it that easy/comfortable to reuse it.

And last point, just like the current sorting was added, the same sorting reversed could be added as well, yet another sorting option.

What bothers me with on this topic is that instead of trying to fix the issues at the correct level we are just either trying to patch or have a DIY syndrome.

Sure, add back the previous checker, but you are going to miss the automatic fixing of isort, etc etc etc. Just think about the idea of modernizing any Products.* package. I'm not the one that will fix all those import unsorted errors, the only effort I would ask anyone to put on that would be to copy&paste p.r.codeanalysis .isort.cfg install isort on a virtualenv and run it in its default mode (i.e. to fix any sorting error), voilà no more sorting errors.

And finally regarding plone.api convention, I really don't care if it's one way or another (at least regarding sorting imports) as long as there is a tool that reports and fixes that by myself I'm totally happy to change the decision every second month, fixing it is just one command and commit away, no more, no less work.

But as I said above, feel free to add it the previous checker back...

[do3cc]

Hi,
you say that my third point is not an issue, because this is a vim integration plugin. "That is not my department" mentality does not help to make things better. If some tool supports much more advanced functionality but does cause trouble for some/many people, there is a tradeoff.
I've seen the isort wiki, but it takes more than two minutes to understand the sort rules from a given isort.cfg. Of course, the valid rules are in the style guides, so this should really not matter.

Regarding .isort.cfg, I've had a second look at django. They use Isort and they have their code checked for compliance. They don't use .isort.cfg, they use setup.cfg. I've tried this and it seems to work.
This could be added to setup.cfg in bobtemplates.plone.

[do3cc]

With the latest changes from @gforcada to flake8-isort there is a warning if not isort settings are configured.
The Plone styleguide has documentation on suggested isort configurations.
Using setup.cfg configuration means, the checker and the editor plugins will always do the right sorting, independent of your cwd being in the root of the project or in the same folder as the source file.
I think this were all errors mentioned in this ticket.
🎉