Don’t allow mixing spaces and tabs for indentation

[eelco]

This extends (and includes) pull request #4312.

Mixing tabs and spaces is not allowed ‘horizontally’ (on the same line) and also not ‘vertically’ (in the same code block).

It’s still allowed in the same file, when used in different code blocks (starting at indentation level 0)

[GeoffreyBooth]

I have to say I love enforced whitespace hygiene, but are we taking on something that should be the responsibility of a linter? Though we sort of validate indentation already as part of detecting new blocks, so perhaps this is something we should get stricter about.

If we do accept this or the other PR, it should be into the 2 branch. This would be a breaking change. I know, it shouldn’t be a breaking change for anyone who has the slightest bit of sanity, but it’s a needless breaking change for anyone who happens to have currently acceptable mixed whitespace a project.

[eelco]

It actually broke one test that seemed to have tabs accidentally, so it’s indeed very likely to break stuff. Also, if you use tabs for indentation but spaces for alignment (as people do), this would be an error.

That said, because whitespace is significant in Coffeescript, I feel strongly that in general this responsibility should not be left to a linter.

[jashkenas]

This is something we certainly could enforce/guide better — but I'm wary of unforseen complications / tricky things that we're not aware of that would be disallowed. I'd also be happier with a more simple implementation, if possible.

Aside from that — looks great! ;)

[GeoffreyBooth]

How does Python handle this?

Also can you please resubmit against the 2 branch? That's where breaking changes should go.

[eelco]

Thanks for the feedback!

@jashkenas What do you find complex about the implementation? I found the current implementation of indentation handling already pretty complex (lot’s of state). To get it simpler it might be worthwhile to try to redo all of it (although I’m not sure that’s in scope for this PR).

@GeoffreyBooth Yes, I changed the base to 2, no need to resubmit. Python 2 handles a tab as 8 spaces (I first proposed something similar in #4303), Python 3 does not allow mixing, but in the style of #4312 (you can still mix on the same line, as long as you do it consistently).

[jashkenas]

@jashkenas What do you find complex about the implementation? I found the current implementation of indentation handling already pretty complex (lot’s of state). To get it simpler it might be worthwhile to try to redo all of it (although I’m not sure that’s in scope for this PR).

Oh — it's probably fine. I just haven't had a chance to really read and understand it. No worries!

[jashkenas]

Would anyone with a big codebase mind giving this patch a try and seeing if it breaks anything for you? With a couple of "worksforme"s I think this can be merged.

[koenbok]

Works for us with the Framer codebase: https://github.com/koenbok/framer

[jashkenas]

Alright then.

[GeoffreyBooth]

@eelco @jashkenas I get a failed test on 2 since this branch was merged in:

failed 1 and passed 724 tests in 2.05 seconds

  test/literate.litcoffee:56:5: error: indentation mismatch
                test "tabbed code", ->
                ^

If I delete everything above this test in literate.litcoffee, leaving just:

Tabs work too:

                test "tabbed code", ->
                    ok yes

Then this test passes.

I guess whatever logic is being used to determine that a new code block has begun, and therefore switching from spaces to tabs between code blocks can occur, is failing for literate CoffeeScript?

[eelco]

Apologies, I have that test ‘fixed’ and a couple more improvements on another branch (based off 1.10.0), I’ll make a pull request soon.

The failing test exposes the complexity with mixing the different whitespace models of Markdown and CoffeeScript and the special treatment tabs and spaces get in the code handling literate CoffeeScript. It’s not an easy (non-opinionated) fix, so the way I ‘fixed’ it was to split the literate CoffeeScript test into one with spaces and one with tabs.

[GeoffreyBooth]

Thanks. Please merge 2 into your branch before you submit the PR.

[GeoffreyBooth]

@eelco do you mind pushing your branch that fixes this test? It troubles me that we have a broken test on the 2 branch.

[jashkenas]

The failing test exposes the complexity with mixing the different whitespace models of Markdown and CoffeeScript and the special treatment tabs and spaces get in the code handling literate CoffeeScript. It’s not an easy (non-opinionated) fix, so the way I ‘fixed’ it was to split the literate CoffeeScript test into one with spaces and one with tabs.

Just to be sure — you're describing a problem with the test, right? And not a problem that this patch created for folks trying to use Literate CoffeeScript?

[GeoffreyBooth]

@jashkenas I think #4336 could create a problem for people using Literate CoffeeScript, if they happen to switch between indenting with spaces for part of the .litcoffee file and indenting with tabs in another part of the same file. For example:

Some nice literate comments . . .

    thisIsCodeIndentedWith = 'spaces'

More comments . . .

    thisIsCodeIndentedWith = 'tabs' # (though GitHub converts it to spaces)

Save this as a .litcoffee file, with the second thisIs line indented with a tab, and it throws an error:

7:2: error: missing indentation
    thisIsCodeIndentedWith = 'tabs'
    ^