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

indentation bug #409

Closed
timblechmann opened this issue Aug 22, 2012 · 15 comments
Closed

indentation bug #409

timblechmann opened this issue Aug 22, 2012 · 15 comments
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. env: SCIDE
Milestone

Comments

@timblechmann
Copy link
Contributor

if a line has a closing bracket, the level of indentation is wrong. current behavior

foo.value(
1.2)

expected behavior:

foo.value(
    1.2)
@jleben
Copy link
Member

jleben commented Aug 23, 2012

what about this:

foo.value(1.2
)

and this:

foo.value(
    1.2).value(
    2.3)

@timblechmann
Copy link
Contributor Author

imo, they should be

foo.value (1.2
    )

and

foo.value (
    1.2).value(
    2.3)

the logic behind is simple: if one opens a bracket, it should increase the indentation level, if one closes, it should decrease.

the second line in the first example should be have an indentation level of 1. in the second example, line 2 should have an indentation level of 1, and line 3 an indentation level of (1-1+1) == 1. (scel indents the third line by 2 levels, but imo, this is wrong)

@jleben
Copy link
Member

jleben commented Aug 23, 2012

Tim, i seem to recall that in Vienna you argued that the second line in the first example should have level 0.

Analogically to the 3rd line in this:

if (a) {
    doSomething
};

@timblechmann
Copy link
Contributor Author

good point. chaning the logic to: if one opens a bracket, it should increase the indentation level, if one closes, it should decrease. if the bracket is the first token of a line, decrease the indentation level of that line.

@jleben
Copy link
Member

jleben commented Aug 23, 2012

but what about:

if (a, {
    doSomething
})

Here only the second closing bracket should decrease indentation, but it's not the first token in the line.
Moreover:

MIDIdef.noteOn(\play, {
    la.la.la
}, 60);

@jamshark70
Copy link
Contributor

Actually, here's the really messy one that Emacs does:

MIDIdef.noteOn(\play, {
    la.la.la
},
    60
);

You can make it a little prettier by hand:

MIDIdef.noteOn(\play,
    {
        la.la.la
    },
    60
);

But if you really want the opening function brace at the end of the first line, it's probably going to look funny no matter what you do:

// huh?
MIDIdef.noteOn(\play, {
        la.la.la
    },
    60
);

@timblechmann
Copy link
Contributor Author

we can probably only do a compromise. maybe if the first token in a line is a closnig bracket, we should only decrease the indentation, if the matching opening bracket triggered an indentation?

@jleben
Copy link
Member

jleben commented Aug 23, 2012

We already do that.
My point is: sometimes (most times?) you'd like the (matching) closing bracket to decrease the indentation even if it's not the first token in its line.

@jamshark70
Copy link
Contributor

I'm not convinced that Jakob's examples really illustrate the point about a second or third closing bracket causing an outdent. You could just as easily say it's the last opening bracket on the earlier line that triggered the indent, in which case it is the first closing bracket that matches.

The problem cases are where you have more than one opener on one line and they are closed on separate lines, or vice versa.

@timblechmann
Copy link
Contributor Author

i've created a gist ... maybe we can try to work on indentation styles there?

https://gist.github.com/3435705

@jleben
Copy link
Member

jleben commented Aug 23, 2012

James, I fail to see your point: are you saying that, even if we look at it differently, the outdent should still happen? In that case we would agree - finally, it doesn't matter how the implementation works, the question is only whether to outdent or not.

@jleben
Copy link
Member

jleben commented Aug 23, 2012

Ah, I think I get it now. The rule could be:

if (the number of closing brackets >= the number of opening brackets on previous line(s))
    if(this line starts with a closing bracket)
        outdent this line
    else
        outdent next line

@jleben
Copy link
Member

jleben commented Aug 23, 2012

OK, I implemented my last proposal. This will satisfy both my MIDIdef and "while" examples, as well as Tim's first example.

@jamshark70
Copy link
Contributor

I just tried a couple of more ambiguous cases after "git pull"ing and they look nice. The first example is just right, two outdent levels for two opening levels. The second -- I don't know if it's possible in any case to make that one look beautiful, but what sc-ide is doing (after your change) is better than Emacs' handling of the identical case.

No complaints with the update, then!

obnoxious[
    contrived.value(
        bracketing_example
)]

// IDE version
obnoxious[contrived.value(
    bracketing_example
    ),
    20
]

// emacs version (no thank you)
obnoxious[contrived.value(
    bracketing_example
),
    20
]

@timblechmann
Copy link
Contributor Author

closing. seems to work great now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Issues that relate to unexpected/unwanted behavior. Don't use for PRs. env: SCIDE
Projects
None yet
Development

No branches or pull requests

3 participants