Skip to content

indentation refactoring #515

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

Merged
merged 38 commits into from
Jul 15, 2016
Merged

indentation refactoring #515

merged 38 commits into from
Jul 15, 2016

Conversation

bounceme
Copy link
Collaborator

No description provided.

@bounceme
Copy link
Collaborator Author

bounceme commented Jun 28, 2016

more testing needed for this, everything is working(except for a few string problems,and generally worse performance, which i hope to fix)

@bounceme bounceme changed the title attempt at indentation refactoring attempt at indentation refactoring:Testers needed Jun 28, 2016
@bounceme bounceme changed the title attempt at indentation refactoring:Testers needed attempt at indentation refactoring:Testing needed Jun 28, 2016
@bounceme
Copy link
Collaborator Author

this is largely finished, if your reading this can you please test? performance suffers within large blocks of code but the simplification makes this a fair trade in my opinion

@markrian
Copy link

I'll test this today, and report here with any regressions or bugs.

@markrian
Copy link

Testing has so far revealed only one thing I'd definitely call a bug, and a couple of other things which are perhaps a matter of personal preference. The former I can only reproduce on a fairly large file, and I'm having trouble producing a minimal test case. In fact, it almost seems like the size is to blame, since changing the file at all seems to resolve the bug. That file is proprietary, so I can't share it, sadly. Once I've managed to make a minimal test case, I'll put it here.

I'll describe the other personal preference "bugs" later, too, when I have some time.

@markrian
Copy link

markrian commented Jun 30, 2016

Bizarrely, it seems that this bug I mentioned only happens when the file I'm working with is 1035 lines or more long. If I remove any line in the file, even blank lines, before, inside or after (?!) the incorrectly indented code, it then indents the whole file correctly.

@bounceme
Copy link
Collaborator Author

bounceme commented Jun 30, 2016

these are caused by the timeout for the search pair function, default is 300 ms. maybe it should be configurable if this gets merged
unfortunately this method requires the very slow search pair function for almost every line indented. the default python and clojure indent scripts work the same but instead of using a timeout they limit the number of lines : https://github.com/neovim/neovim/blob/master/runtime/indent/python.vim#L58-L59
. sadly, there is a incredibly fast way, but it can break in some cases due to the differences between c and js strings

@bounceme
Copy link
Collaborator Author

i have a few ideas for improving the speed issue, 300 ms is too much(though it would only take that long in a very big block scope)

bounceme added 8 commits July 1, 2016 10:52
this limits the amount of matches search pair finds, this regex needs work
I think this is good to merge to develop for further testing and speed improvements
@bounceme bounceme changed the title attempt at indentation refactoring:Testing needed indentation refactoring Jul 2, 2016
@bounceme
Copy link
Collaborator Author

bounceme commented Jul 2, 2016

@markrian there have been improvements to this, is this working any better for you?

@markrian
Copy link

markrian commented Jul 3, 2016

I'll take a look tomorrow. Sounds good!

@amadeus
Copy link
Collaborator

amadeus commented Jul 6, 2016

So this sounds like it might be an improvement over develop then. I'll try and test it out soon.

@bounceme
Copy link
Collaborator Author

bounceme commented Jul 8, 2016

I think this is about done, if you see any regressions let me know. also i am glad to hear any suggestions on what i've tried here

@bounceme
Copy link
Collaborator Author

bounceme commented Jul 8, 2016

I'll merge later today or tomorrow if its alright with @amadeus & @markrian

@markrian
Copy link

markrian commented Jul 9, 2016

I'm afraid I've found another indenting bug. Trying to produce minimal test case.

@markrian
Copy link

markrian commented Jul 9, 2016

It seems to be another idempotency issue. The follow correctly indented code stays intact after one gg=G:

'use strict';

function foo() {
}

but a second call results in this:

'use strict';

    function foo() {
    }

and subsequent calls indent the function further and further.

@markrian
Copy link

markrian commented Jul 9, 2016

Just found another one. Do/while loops mess up indentation of the line immediately following them:

function foo() {
    do {
        anything();
    } while (false)

         anything();
    anything();
}

@bounceme
Copy link
Collaborator Author

bounceme commented Jul 9, 2016

i think the first issue is fixed but the do while syntax makes it very unlikely that can be solved unless there is a do block syntax element added, which may or may not be possible

@bounceme
Copy link
Collaborator Author

bounceme commented Jul 9, 2016

@bounceme
Copy link
Collaborator Author

been on vacation this past week, hopefully someone was testing this as I'd like to finish and merge it tomorrow

@amadeus
Copy link
Collaborator

amadeus commented Jul 15, 2016

Overall, things have been pretty good, however I have found a couple weird instances, mostly working within JSX files, not sure if they are related to JSX, or if it's something deeper with JS.

I'll see if I can isolate the problems over the next couple days.

@amadeus
Copy link
Collaborator

amadeus commented Jul 15, 2016

However, at this point, it may be worth merging this in and then working to fix the rest of the issues over time? Also getting it into develop will inevitably get more eyes on it and therefore more issues may be found to get fixed.

@bounceme
Copy link
Collaborator Author

Sounds fine, i'll merge after a few finishing touches

@bounceme bounceme merged commit 32e7c78 into develop Jul 15, 2016
@bounceme bounceme deleted the indent-refactor branch July 15, 2016 07:53
@amadeus amadeus mentioned this pull request Jul 17, 2016
@vxsx vxsx mentioned this pull request Sep 6, 2016
@bounceme bounceme mentioned this pull request Nov 8, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants