pangloss / vim-javascript

Vastly improved Javascript indentation and syntax support in Vim.
http://www.vim.org/scripts/script.php?script_id=4452
3.8k stars 358 forks source link

Indentation issue #683

Closed sassanh closed 8 years ago

sassanh commented 8 years ago

Is it an issue or am I missing something:

if (false ||
    true) {
        do_something();
    }

Last 2 lines seems to have extra indentation.

bounceme commented 8 years ago

https://github.com/pangloss/vim-javascript/pull/621#issuecomment-244585305

you've happened to ask that like 4 times now

sassanh commented 8 years ago

I'm absolutely sure it was fixed. Btw you said here https://github.com/pangloss/vim-javascript/pull/621#issuecomment-244530847:

thats unrelated, lines that start with }]) are indented to the line which contains the other pair. wontchange

Then I guess we should change the logic. The result is just no right, C++ indenter in vim doesn't indent like this too: C++:

int main() {
    if (true and
            false) {
        cout << 123;
    }
}

If } indents to the other pair then I guess we should just change it. Tell me you don't have time but you agree with me and I'll manage to do this or maybe we decide to just ignore it. But "it's correct the way it is just because it's our current implementation" doesn't make sense.

sassanh commented 8 years ago

@bounceme Btw, I'm sorry if my issues annoy you. I'm absolutely thankful for your job here. If it's the case that you find these issues irrational or annoying just tell me and I'll try to find another way for my problems.

bounceme commented 8 years ago

It's correct. I would keep the behaviour as it is even if someone submitted a p.r.

int main() {
  if (true and
    false) {
      cout << 123;
    }
}

this is our behaviour and I find it easier to understand. Also the possibility of this being added without serious performance loss is slim

bounceme commented 8 years ago

https://github.com/pangloss/vim-javascript/pull/684

bounceme commented 8 years ago

I still don't like it compared to the current behaviour though. It is a matter of preference and code complexity

bounceme commented 8 years ago

I'm absolutely sure it was fixed.

Would I not notice making this 'fix' myself?

sassanh commented 8 years ago

Thanks for that commit, it fixes the brace but not the content. OK, what about a setting variable for it? So that user can decide to have it the way he prefers based on preference and his machine's resources. Even though it's hard to understand for me why you prefer it the way it is, I guess you agree that the one I'm looking for is a valid at-least-good indentation that maybe less or more around %50 of users prefer it that way. Maybe it deserves a setting variable.

Would I not notice making this 'fix' myself?

I tried to find the commit that it was fixed in but didn't find it. Probably I'm mistaken. I don't know I just indent whole file I'm working on once in few hours and never noticed this in last month. Maybe I noticed it after seeing the jsx problem and then looking more carefully.

bounceme commented 8 years ago

I can only see that happening if my opinion were to change on that indentation. another massive thread discussing what is preference is not going to change that

sassanh commented 8 years ago

Btw, I guess eslint is another good reference, run eslint --fix on a file containing above code while you have indent rule activated in your .eslintrc, this is the result with "indent": ["error", 2]:

if (true &&
    false) {
    1;
}
sassanh commented 8 years ago

another massive thread discussing what is preference is not going to change that

OK, if it won't change it let it be. I'll find a way to solve my problem.

sassanh commented 8 years ago

Actually it's already solved, neomake + eslint --fix seems to solve all my indentation problems as it just indents the way I expect.

bounceme commented 8 years ago

Alright, I've sided with you on this

https://github.com/pangloss/vim-javascript/pull/684

sassanh commented 8 years ago

Oh thanks. But content is still not indented. I guess it's still work in progress.

bounceme commented 8 years ago

refer to our previous conversations for wy the content won't be changed to be indented