MDAnalysis / mdanalysis

MDAnalysis is a Python library to analyze molecular dynamics simulations.
https://mdanalysis.org
Other
1.32k stars 651 forks source link

Long-term solution for import sorting #3656

Open PicoCentauri opened 2 years ago

PicoCentauri commented 2 years ago

Is your feature request related to a problem?

As discussed in #3644 I am always frustrated when I see unsorted imports πŸ™ƒ. Especially, for a new import, it is not clear where to put it.

Describe the solution you'd like

Run isort with options (to be discussed) for the complete repo

line_length = 80
indent = 4
multi_line_output = 8  # Vertical Hanging Indent Bracket
include_trailing_comma = true
lines_after_imports = 2
known_first_party = "MDAnalysis"

and add a command like isort --verbose --check-only --diff to the CI.

Describe alternatives you've considered

Leave everything as it is.

IAlibay commented 2 years ago

My 2 cents is that I really don't like things that automatically reformat code we write. This is purely a case of "I know better than the machine", is there a bot that can tell us off instead of making this a pre-commit hook?

PicoCentauri commented 2 years ago

If it is in the CI it is not automatically sorted. We just get informed what and how to resort.

IAlibay commented 2 years ago

yeah I can live with it being just a CI warning. Will it apply purely on the diff?

PicoCentauri commented 2 years ago

I don’t know if you can do this. The pure diff is most likely not possible maybe the touched files...

On 26 Apr 2022, at 16:50, Irfan Alibay @.***> wrote:

yeah I can live with it being just a CI warning. Will it apply purely on the diff?

β€” Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.

IAlibay commented 2 years ago

touched files would be fine, essentially if we can avoid it screaming at us every time about known cases where we've agreed we don't want to deal with it (assuming those happen, I'm thinking converters might be such a case), that'd be great.

PicoCentauri commented 2 years ago

Probably something like

TOUCHED_FILES=$(git diff --name-only develop)
isort --verbose --check-only --diff $TOUCHED_FILES

will do the job...

RMeli commented 2 years ago

if we can avoid it screaming at us every time about known cases where we've agreed we don't want to deal with

You could flag such cases with action comments.


Related to #2450