use_tabs=True does not work for lists and dictionaries

eskhool commented 7 years ago

We use_tabs for indentation but continuation_indent_width still uses spaces instead of tabs for lists and dictionaries that are split over multiple lines. If required there should be a separate knob for this which allows setting of tabs vs spaces for lists and dictionaries

``

bwendling commented 7 years ago

I'm not a "tabs" guy, so I need to ask more about what you need. Should the tabs in the continuation indent be the same number as the value of the CONTINUATION_INDENT knob? So if it's 4, then there would be 4 tabs?

eskhool commented 7 years ago

Thanks for not starting a 'holy war' on this 👍 . Ideally, though this may vary even amongst us 'tabs' people, I think I would want the continuation indent to be additional to the indent at which the code is at right now. so CONTINUATION_INDENT=1 should result in the code below Eg below

def a():
    really_long_dictionary_name = {
        first_really_long_key: 1,
        second_really_long_key: 2,
    }

bwendling commented 7 years ago

I'm not above holy wars, but we need to support the feature. ;-)

I think I understand what you want then. I'm going to create a branch and let you test it out for me, if you would.

eskhool commented 7 years ago

Us minority of 'tabbers' definitely need the support of the larger community :D. Happy to test, with a reasonably large and varied code base.

yota-code commented 7 years ago

I agree with @eskhool: an opening character (brace, bracket, etc.) give me an extra indent (ie. tab) on the next line. As a "tabber" too, I can not understand the trend to do ASCII-art and space based alignment :), where you break everything each time you change a name...

Nonetheless, thank you for yapf

eskhool commented 7 years ago

@yota-code, as a fellow tabber ...are you able to use yapf at all without this change? I find that it completely breaks my formatting. Just clarifying if that thank you if for the effort in general or because it works for you also :)

LEW21 commented 7 years ago

+1 about just using extra normal indent, as a rule there is simply no place in the tab world for visual indents.

@eskhool I'm not.

LEW21 commented 6 years ago

Actually, it seems that it works correctly on yapf 0.16.1+. At least with the following config

[style]
based_on_style = pep8
column_limit = 9999
dedent_closing_brackets = true
use_tabs = true
indent_width = 1
continuation_indent_width = 1

PetterS commented 6 years ago

I think using tabs only for indentation is also a common case. For example, this is supported by clang-format as "UseTab: ForIndentation". Then spaces can still be used to vertically align stuff.

gregwym commented 6 years ago

Seems like it will be part of v0.20.2. Very exciting to see this lands!

gregwym commented 6 years ago

Just tired the version from master branch with change from https://github.com/google/yapf/commit/d377549f52c46885b9987b2e611fa63b5c99eb99. While this fixed the leading indents to use tab, the inner indents are still spaces. You can see from this picture.

bwendling commented 6 years ago

If they're not spaces though all of the code alignment will fail. Is that the correct behavior? (I'm not a tab user...)

gregwym commented 6 years ago

@gwelymernans From the practice I have seen and people's example above, the inner indents should just be 1 tab regardless of alignment. Is there a specific example that requires space instead? Thank you for making this happen! Happy to help testing as well.

PetterS commented 6 years ago

The inner indents have to be spaces. clang-format does this as well. As Bill said, this is required for alignment.

On Sat, Feb 3, 2018, 02:17 Greg Wang notifications@github.com wrote:

@gwelymernans https://github.com/gwelymernans From the practice I have seen and people's example above, the inner indents should just be 1 tab regardless of alignment. Is there a specific example that requires space instead? Thank you for making this happen! Happy to help testing as well.

— You are receiving this because you commented. Reply to this email directly, view it on GitHub https://github.com/google/yapf/issues/380#issuecomment-362759003, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvI0Q7KOsbX86fiGhKniiTP7DcFW3jjks5tQ7OUgaJpZM4MX7RG .

gregwym commented 6 years ago

@PetterS clang-format does this doesn't means it is correct in python. According to PEP8 E101 and E111 indentation should not mix spaces and tabs and indentation should always be multiple of four (or 1 tab).

code	sample message
E101	indentation contains mixed spaces and tabs
E111	indentation is not a multiple of four

Could you give an example where using space is required for alignment?

PetterS commented 6 years ago

(There is also a PEP8 warning if tabs are used at all for indentation. Many style guides, including the one used by yapf, do not follow PEP8.)

Examples where this is required are abundant. Here is one: https://github.com/google/yapf/blob/f5fd454be546bdbb90dd5e345771495475f33713/yapf/yapflib/pytree_utils.py#L293

Of course, never vertically aligning line continuations is also a possibility. But vertical alignment is currently used in yapf and that requires spaces.

On Mon, Feb 5, 2018 at 9:00 PM Greg Wang notifications@github.com wrote:

@PetterS https://github.com/petters clang-format does this doesn't means it is correct in python. The very first pep8 error code E101 is "indentation contains mixed spaces and tabs". Could you give an example where using space is required for alignment?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/yapf/issues/380#issuecomment-363203557, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvI0QkAG6zQpEEo73FKHAYjJDrAe3qPks5tR13SgaJpZM4MX7RG .

gregwym commented 6 years ago

My understanding is, {based_on_style: pep8, use_tabs: true} means we follow all rules in pep8 except W191: indentation contains tabs. So maybe we can consider to drop vertical alignment when using tab? Or have it as a separate knob, which means user also need to ignore E101 and E111 in their linter.

With space alignment:

With tab and no vertical aligning:

PetterS commented 6 years ago

Those two solutions are both reasonable.

On Thu, Feb 8, 2018, 01:20 Greg Wang notifications@github.com wrote:

My understanding is, {based_on_style: pep8, use_tabs: true} means we follow all rules in pep8 except W191: indentation contains tabs. So maybe we can consider to drop vertical alignment when using tab? Or have it as a separate knob, which means user also need to ignore E101 and E111 in their linter.

With space alignment: [image: image] https://user-images.githubusercontent.com/510089/35948620-45558b8a-0c22-11e8-92d3-cd2e33e2384d.png

With tab and no vertical aligning: [image: image] https://user-images.githubusercontent.com/510089/35948639-63f0d978-0c22-11e8-807b-2e83bf35976d.png

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/yapf/issues/380#issuecomment-363958502, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvI0eMrW0XqK0IQTzeAhv9wl-43rOTCks5tSj3LgaJpZM4MX7RG .

bwendling commented 6 years ago

Are you sure the E101 error means that there are spaces after a tab (used for continuation indentations like here), or that you have something like: \t \t?

The meaning of the USE_TABS knob is meant to try to accommodate tab users. :-) However, I like the idea of a separate knob.

@gregwym From your example, it would appear that a continuation alignment that uses tabs would be one tab per indentation level. Is that correct? What about things like:

if (some != weird and
    (code >= that.goes() or
     on_separate_lines)):
    pass

Or are you talking specifically about when indenting lists / dictionaries / other containers?

gregwym commented 6 years ago

Yes, E101 also apply to continuation indentations. There isn't space mixed within tabs like \t \t in my example as you can see all invisible chars in the screenshots. (I'm using Visual Studio Code.) One tab per indentation level is the way to go.

For your example, we can format like this.

Or like this, without enclosing bracket.

But not with space :(

The behavior of current implementation in master is also weird for condition without enclosing bracket:

eskhool commented 6 years ago

Sorry to spam everyone but @gregwym...what IDE/editor is this? Never mind...just read that its Visual Studio Code...

PetterS commented 6 years ago

Greg literally wrote this in the last message (VS code).

On the topic of this issue, I think it is strange that E101 reports spaces used for vertical alignment. Vertical alignment is used in PEP8 and there is no reasonable way of aligning things vertically like that without tabs. There is really two types of indentation in programming: scope indentation and indentation used for line continuation.

Other style guides (not PEP8) require not using vertical alignment, because this can a single line change to have a "blast radius" to other lines. Having a setting to use only a single tab for line continuation is not unreasonable.

Personally, I am happy with the current functionality.

On Fri, Feb 9, 2018 at 1:07 PM Anshuman Aggarwal notifications@github.com wrote:

Sorry to spam everyone but @gregwym https://github.com/gregwym...what IDE/editor is this?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/google/yapf/issues/380#issuecomment-364416606, or mute the thread https://github.com/notifications/unsubscribe-auth/AAvI0dJu7DAV2syvyK4ueLneYgGvu50aks5tTDTkgaJpZM4MX7RG .

bwendling commented 6 years ago

@gregwym Thanks for the examples. I have some further questions now. When we add tabs for indentation (as opposed to continuations), it's based off of the "indent level". However, when we have something like my example above, it looks like for the first continuation indent you add a tab. And then if there's another continuation (the line with on_separate_lines) you add two tabs. Is this correct? And is it a universally applicable rule? The issue is that we don't have an indent level to base how many tabs to add off of.

Another question has to do with function formatting:

this_is_a_function_with_a_lot_of_args(arg1, arg2, arg3,
        arg4, arg5, arg6)

If YAPF splits the function like so, then is there just one tab before arg4? Or are there enough tabs to make it align with arg1?

There might be more questions, but these are two off the top of my head. :-) Basically, if there's a universal rule I can follow, that would be golden.

eskhool commented 6 years ago

@gwelymernans, there are 2 basic school of thoughts (amongst us tabbers=tab users) for continuation alignment

Tab only (for both indentation and alignment) In this case whenever continuing on a long line, there will be an additional tab like in the 1st screenshot of @gregwym
Tab and space indentation (tab for code indentation and tab+spaces for alignment) In this case the tab indentation used for the primary line is retained and further indentation is done through spaces. This keeps the code aligned regardless of the tab to space display being used on a particular display/IDE and works well for those who use it.

Currently yapf does a very unexpected switchover back to pure spaces for continuation indentation which is probably completely invalid for any tabber.

If yapf has to truly cater to tab users, it will probably have to have a flag for those of us in the 2nd school of mixed indentation

hope I have helped

yinyin commented 6 years ago

I bump into mix-space-tab issue recently and try to resolve it by adding a knob CONTINUATION_ALIGNMENT_TYPE to switching between known styles. Here is the branch of my approach: valign-for-tabber-2.

The following alignment modes are implemented: (I failed to have good mode names ... suggestion is welcome)

SPACE: vertically align with spaces. current (0.20.2) and default behavior.
FIXED: just use fixed number of tabs (CONTINUATION_INDENT_WIDTH / INDENT_WIDTH).
LESS: Slightly left (less tab) if cannot vertically align continuation lines with tabs.
MORE: Slightly right (more tab) if cannot vertically align continuation lines with tabs.

Here is how different modes look like:

with configuration:

[style]
based_on_style = chromium
USE_TABS = true
COLUMN_LIMIT=32
INDENT_WIDTH=4
CONTINUATION_INDENT_WIDTH=8
# CONTINUATION_ALIGNMENT_TYPE = space
# CONTINUATION_ALIGNMENT_TYPE = mixed
# CONTINUATION_ALIGNMENT_TYPE = less
# CONTINUATION_ALIGNMENT_TYPE = more

Would this approach viable for this issue ?

PetterS commented 6 years ago

SPACE and FIXED are reasonable options. LESS and MORE just don’t make sense to me. When would that ever be useful? The major point of using tabs is that their width can be customized in the editor.

gregwym commented 6 years ago

Sorry for getting back on this late. @eskhool summarized it well and I think @yinyin 's solution for SPACE and FIXED will cater both flavors of tab user well.

yinyin commented 6 years ago

It happens to me that some code base I am working on is formatted in MORE style.

The story behind this is:

We work with customer on their application.
The code base was indent with space at first.
The customer have difficulty in integrating development with tools like linter.
Sometimes they accidentally indent with 3 spaces.
Switch to tab (and have every editor's tab width preset to same value) to avoid such accident.

ichibrosan commented 5 years ago

At the risk of sounding like I want to be part of a holy war, as a python user, the thought of using spaces disturbs me because indentation errors break python code really quickly, and a four space tab is visually recognizable. I am trying to be progressive and follow Googles style guide, and use pylint and yapf. I use Wing IDE Pro. The tools are fighting each other and I am not finding middle ground. Any suggestions would be appreciated.

ichibrosan commented 5 years ago

I decided to get with PEP 8 and switch to spaces. It feels wrong/strange but I need to go with the flow here. So I reconfigured my IDE to use spaces, and yapf is off my bad list. Adapt or die. :-)

eskhool commented 5 years ago

I decided to get with PEP 8 and switch to spaces. It feels wrong/strange but I need to go with the flow here. So I reconfigured my IDE to use spaces, and yapf is off my bad list. Adapt or die. :-)

@ichibrosan, I think you mean conform or die, I say NEVER :-)

alexreg commented 3 years ago

As a die-hard tabber, I extend my thanks for implementing this feature!

Two minor points: a) Should continuation_align_style = fixed by default when use_tabs = true, perhaps? b) It's a bit odd that continuation_indent_width = 2 by default when use_tabs = true, even if it agrees with the default of eight spaces under under the "4 spaces = 1 tab" convention.

Those minor points aside, this issue can probably be closed, I'd think.

eskhool commented 2 years ago

@yinyin Thanks for the good work. I see that it has made its way via CONTINUATION_ALIGN_STYLE in the code. However it fails when the line continuation \ is used to split a long line

google / yapf

use_tabs=True does not work for lists and dictionaries #380