Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add multi_line_output mode for more compact formatting #941

Closed
gwk opened this issue May 6, 2019 · 15 comments · Fixed by #1344
Closed

Add multi_line_output mode for more compact formatting #941

gwk opened this issue May 6, 2019 · 15 comments · Fixed by #1344
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed

Comments

@gwk
Copy link

gwk commented May 6, 2019

Could we add a mode similar to 0/grid mode, but that uses the fixed indent width for subsequent lines? Mode 0 looks like this:

from third_party import (lib1, lib2, lib3,
                         lib4, lib5, ...)

I would like this (where indent=2):

from third_party import (lib1, lib2, lib3,
  lib4, lib5, ...)

This would be more compact, and can reduce line count. I would be happy to attempt a PR; pointers on where to look would be welcome. isort has been a great improvement to my workflow; thank you for building it!

@julie777
Copy link

FYI, the current mode 0 matches the pep8 style guide whereas the suggestion for indent does not.

@timothycrosley timothycrosley added enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed labels Jan 7, 2020
@timothycrosley
Copy link
Member

While I don't have the bandwith to add support for this requested feature myself, I will accept a pull request that adds it. The correct place to add this functionality is here: https://github.com/timothycrosley/isort/blob/develop/isort/wrap_modes.py#L46

@gwk
Copy link
Author

gwk commented Jan 10, 2020

Thanks @timothycrosley. I'll try to find time to take a stab at this. Line 36 says that name and order are significant; should I add a new @_wrap_mode decorated function to the end, after noqa?

@timothycrosley
Copy link
Member

@gwk,

Glad to hear! Look forward to the pull request. Yes that's correct, right after noqa.

Thanks!

~Timothy

sztamas added a commit to sztamas/isort that referenced this issue Jul 23, 2020
@sztamas
Copy link
Collaborator

sztamas commented Jul 23, 2020

Hi there,

I looked into this one a bit and it can be almost achieved with the HANGING_INDENT or 2 mode. The only problem is that HANGING_INDENT hard-codes the usage of \ and ignores the --use-parentheses completely.

This problem seems to occur in the other modes as well, but the other way around ie. most of them hard-code the usage of parentheses.
I'm guessing that people don't notice this, because they probably use a profile and the profiles tend to use use_parentheses: True or they just prefer using parentheses. However, turning off use_parentheses isn't possible for most modes.

I made some modifications in my fork that makes HANGING_INDENT mode "respect" the --up option.
With this change, and an indent of 2, you can achieve what the OP wanted:

$isort -d test.py -m HANGING_INDENT -i 2 --up
from third_party import (lib1, lib2, lib3, lib4, lib5, lib6, lib7, lib8, lib9, 
  lib10, lib11, lib12, lix1, lix2, lix3, lix4, lix5, lix6, lix7, lix8, lix9, 
  lix10, lix11, lix12)

Please let me know what you think about this approach.

sztamas@c74041e

If it is OK, I would prefer to look at the tests covering this area and add some additional ones and/or update some to reflect the changes.
It is also probably a good idea to update the other modes to use backslash-joining by default and use parentheses only if use_parentheses is True. Would you agree?
Maybe that could be done as a separate issue and PR?

Thanks,

Tamas

@gwk
Copy link
Author

gwk commented Jul 23, 2020

This looks like what I was hoping for. My only question is, does this insert parentheses unconditionally or only when the line needs to wrap?

@timothycrosley
Copy link
Member

Will put more details here later, but, the use_parenthesis is not meant for wrap modes but only meant for single line wrapping with one import. isort has a release gaurentee that the same options will return the same output, other than fixing large errors. We could think about changing that in the future for 6.x.x but for the meantime code can be refactored to reuse logic, but a different wrap mode rather then utilizing the --up would be needed for this ticket

@timothycrosley
Copy link
Member

timothycrosley commented Jul 24, 2020

Some additional information:

Here is the official release policy: https://timothycrosley.github.io/isort/docs/major_releases/release_policy/#formatting-guarantees
Here is the updated documentation on the use_parentheses option to clarify its intended and effective usage: https://timothycrosley.github.io/isort/docs/configuration/options/#use-parentheses
If you look at where it's used, it only gets used for individual line wrapping, but never effects wrap modes: https://github.com/timothycrosley/isort/search?q=use_parentheses&unscoped_q=use_parentheses. I'd also note that profiles are very new, but this behaviour has existed for a few years now.

Thanks!

~Timothy

@sztamas
Copy link
Collaborator

sztamas commented Jul 24, 2020

Hi @timothycrosley

All that makes a lot of sense.

What would be a good name for the new wrap mode?
I assume it will have to be added as the last mode to keep things backward compatible.

As a side note, it might be just me, but the mode names don't really tell me what the difference is between them.
Wouldn't it make sense to have a generated doc page when you show the modes with some examples to see what the difference is without having to try them out one-by-one?

Thanks,

Tamas

@sztamas
Copy link
Collaborator

sztamas commented Jul 24, 2020

This looks like what I was hoping for. My only question is, does this insert parentheses unconditionally or only when the line needs to wrap?

Hi @gwk

It inserts the parentheses just when the lines wraps.

Thanks,

Tamas

@timothycrosley
Copy link
Member

What would be a good name for the new wrap mode?

Open to any name that makes sense, your initial observations make me think HANGING_INDENT_WITH_PARENTHESES might be a good candidate - though it is very verbose.

I assume it will have to be added as the last mode to keep things backward compatible.

Yes that's correct! If that assumption isn't commented in the source file via comments and/or via tests, another improvement that could be made is clarifying it.

As a side note, it might be just me, but the mode names don't really tell me what the difference is between them.
Wouldn't it make sense to have a generated doc page when you show the modes with some examples to see what the difference is without having to try them out one-by-one?

💯 % This. It would be really nice to have autogenerated docs, under the config tab of the new documentation website. Maybe pick a long example multi import statement and auto run it through all modes. Again this is just a bonus improvement that could be done.

Thanks for all the thought put into this!

~Timothy

@sztamas
Copy link
Collaborator

sztamas commented Jul 24, 2020

I like HANGING_INDENT_WITH_PARENTHESES and although is long, there is precedent, it comes only as shared 🥈 😄

VERTICAL_PREFIX_FROM_MODULE_IMPORT
VERTICAL_HANGING_INDENT_BRACKET
HANGING_INDENT_WITH_PARENTHESES
VERTICAL_GRID_GROUPED_NO_COMMA
VERTICAL_HANGING_INDENT
...

We have the numeric short-hands OFC, maybe worth introducing short-hands based on the first character of the modes that have more than 1 word? Would be similar to what you have for normal options already, backward compatible and there are no conflicts ATM:

 0, GRID
 1, VERTICAL
 2, HANGING_INDENT, HI
 3, VERTICAL_HANGING_INDENT, VHI
 4, VERTICAL_GRID, VG
 5, VERTICAL_GRID_GROUPED, VGG
 6, VERTICAL_GRID_GROUPED_NO_COMMA, VGGNC
 7, NOQA
 8, VERTICAL_HANGING_INDENT_BRACKET, VHIB
 9, VERTICAL_PREFIX_FROM_MODULE_IMPORT, VPFMI
10, HANGING_INDENT_WITH_PARENTHESES, HIWP

That's another idea that can result in another issue/PR though 😄 , going back to this issue there is still one related problem left.
The current hanging indent mode doesn't take --tc into consideration. That must be a bug!

Am I fixing it while I'm at it or would you prefer that to be separated into another separate issue?

Thanks,

Tamas

sztamas added a commit to sztamas/isort that referenced this issue Jul 25, 2020
Adds a new wrap mode Hanging Indent, With Parentheses (10).

Fixes PyCQA#941.
@sztamas
Copy link
Collaborator

sztamas commented Jul 25, 2020

OK, I think this is done in the PR linked above.

Usage:

$ isort -d test.py -m HANGING_INDENT_WITH_PARENTHESES -i2 
from third_party import (lib1, lib2, lib3, lib4, lib5, lib6, lib7, lib8, lib9,
  lib10, lib11, lib12, lib13, lib14, lib15, lib16, lib17, lib18, lib19, lib20,
  lib21, lib22, lib23, lib24, lib25, lib26, lib27, lib28, lib29, lib30)

I think wrap_modes.py could be refactored a bit, to remove some of the duplication and make the intent a bit clearer, but I refrained, in order to get the new feature in.

@timothycrosley
Copy link
Member

This was just deployed with version 5.2.0 of isort (thanks @sztamas!).

~Timothy

@gwk
Copy link
Author

gwk commented Sep 14, 2020

@sztamas & @timothycrosley, I just want to say thank you for implementing this. I finally got back to working on my library and this did the trick!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants