Skip to content

adjustable indent (experimental) #621

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 10 commits into from
Sep 4, 2016
Merged

adjustable indent (experimental) #621

merged 10 commits into from
Sep 4, 2016

Conversation

bounceme
Copy link
Collaborator

@bounceme bounceme commented Sep 2, 2016

mxw/vim-jsx#116 (comment)
thanks to @mxw for the suggestion, it is slightly less useful than the cindent version though

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

@sassanh please test this out, ive made the changes you wanted

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

here is a small demo
https://asciinema.org/a/3j66lltfis3p0mhhtgnycpz48

@sassanh
Copy link

sassanh commented Sep 3, 2016

@bounceme thank you, that was fast. It's alright overall though there are some minor issues. Look at this:

const a = <div
    a={1 +
            2 +
            3 +
            4 +
    }
    b={true
            ? 1
            : 2
    }
    c={true
            ? 1
            : 2 && (true &&
                    false &&
                    false &&
                    true) && (true &&
                            false &&
                            false &&
                            true) && (
                                true &&
                                    false &&
                                    true
                            )
    }>123
</div>;

While rest of expressions are reasonable and can be considered they're following a rule, last parenthesis is inconsistent.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

true) && (
                                true &&
                                    false &&
                                    true
                            )

that is because a continuation is started. the previous line ended in a parenthesis.
sorry the formatting is gross(in my example, 8sw)
edit: just messed up the pasting actually hah

@mxw
Copy link

mxw commented Sep 3, 2016

What's a "continuation"? Is it a JS construct I'm not aware of?

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

not really, :h cino-+
'a line that spills over into the next'

@mxw
Copy link

mxw commented Sep 3, 2016

I agree with @sassanh that

var foo = (
    true &&
        false &&
        false
);

is not a "correct" indentation—I don't know of any style guidelines (save perhaps the most bizarre Python styles) where this is recommended. I think I empathize more with the difficulties of trying to maintain a cobbled-together pseudo-parser—but I don't think that implementation details are a good justification for behavior.

@mxw
Copy link

mxw commented Sep 3, 2016

(I don't know the vim-javascript codebase at all, though, so for all I know this is prohibitively difficult to implement.)

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

https://github.com/pangloss/vim-javascript/blob/master/indent/javascript.vim#L83 is the extent of this 'pseudo parser' . I won't be removing line continuation, though you can with the config variables https://github.com/pangloss/vim-javascript/blob/master/indent/javascript.vim#L63

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

It could be changed to only happen in blocks(not in object literals, arrays, args), as with cindent

@sassanh
Copy link

sassanh commented Sep 3, 2016

This is another issue I found with this branch:

                <X
                    itemKey='id'
                    template={props =>
                            <QuestionTag {...props.item} readonly={true}/>}
                            callback={(event, _, __, items) => }
                />

callback should be indented as same level as template.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

are you sure that isn't an issue with jsx only?

@sassanh
Copy link

sassanh commented Sep 3, 2016

It happened after I checked out this branch, I'm sure it was working correctly in develop and jsx branches.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

as this is the javascript repo, could you please reduce this to a reproducible js only example?

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

#621 (comment)
That is something I actually will do

@sassanh
Copy link

sassanh commented Sep 3, 2016

I just hope after you're done with it, it doesn't affect experience with vim-jsx if it doesn't improve it.

@sassanh
Copy link

sassanh commented Sep 3, 2016

Another issue:

if (true &&
    false) {
        const a = 1;
    }

last line should have indent width 0.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

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

@sassanh
Copy link

sassanh commented Sep 3, 2016

I see, you're right, it happens in develop branch as well.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

#621 (comment)
is fixed

@sassanh
Copy link

sassanh commented Sep 3, 2016

Very nice, this latest commit made it much better. I continue testing and will share any problem I find here.

@sassanh
Copy link

sassanh commented Sep 3, 2016

A minor issue:

const a = true
    ? {
        zIndex: '1',
        backgroundColor: this.props.selected
        ? '#a0c4da'
        : '#cccccc',
        border: '1px solid #999999',
    }
    : {};

The ternary if in backgroundColor is not indented at all. Though I'm totally cool with its current indentation just thought it helps to report it.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

that contradicts #621 (comment)
#621 (comment)

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

is that a github bug? ignore that, i thought comments were numbered for a second

@sassanh
Copy link

sassanh commented Sep 3, 2016

I see, it's alright the way it is.

@sassanh
Copy link

sassanh commented Sep 3, 2016

I don't know about that, I don't think so.

But look at this:

componentDidMount() {
        ::this.fetch();
}

double indent.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

fixed

@sassanh
Copy link

sassanh commented Sep 3, 2016

Didn't find any other issues with this branch. Checked it with ~2000 lines of js and jsx code.

@sassanh
Copy link

sassanh commented Sep 3, 2016

A really minor issue is it somehow tries to indent commented lines too :D

                {/* <div style={styles.y}> */}
                    {/*     <Z */}
                        {/*         {...this.props.x} */}
                        {/*         readonly={true} */}
                        {/*     /> */}
                    {/* </div> */}
                {/* <div style={styles.y}> */}
                    {/*     <Z */}
                        {/*         {...this.props.x} */}
                        {/*         readonly={true} */}
                        {/*         resource='question-label' */}

But I guess it's not relevant to these commits.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

that is an issue with mxw/vim-jsx isn't it? i can't reproduce with ft=javascript

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 3, 2016

since they are so prone to user error, and they aren't so needed now I'll add a deprecation warning in the readme

@bounceme bounceme merged commit 7e6d963 into develop Sep 4, 2016
@bounceme bounceme deleted the adj branch September 4, 2016 01:31
@sassanh
Copy link

sassanh commented Sep 4, 2016

I found an issue:

if (prevProps.x !==
    this.props.y) {
    this.setState({
        x: 1
    });
    }

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 4, 2016

#621 (comment)

@sassanh
Copy link

sassanh commented Sep 4, 2016

Oh, right, my bad.

@sassanh
Copy link

sassanh commented Sep 4, 2016

What about this:

if (true &&
    false) {
    a({
        1: 2,
    });
        b = 1;
    }

b=1 has wrong indentation.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 4, 2016

my results:

if (true &&
  false) {
    a({
      1: 2,
    });
    b = 1;
  }

@sassanh
Copy link

sassanh commented Sep 4, 2016

Yeah I got latest develop version and it's fixed.

@sassanh
Copy link

sassanh commented Sep 4, 2016

Are you using a variable to keep some state for indentation? After using it a while it will start indenting things like this #621 (comment) Like indenter didn't clean a variable or something like that.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 4, 2016

Yes, though it will never do things like that because it is used only in very specific circumstances. I did just fix something though which would cause some problems with the cache

@sassanh
Copy link

sassanh commented Sep 4, 2016

OK, I will tell you if I see it again.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 4, 2016

yea, only versions after 22d0e64 had the problem

@sassanh
Copy link

sassanh commented Sep 4, 2016

Nice, it's much more stable after your latest commits. I can't even find minor issues. Good job.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 5, 2016

If there were any jsx bugs found here, you might report them at mxw/vim-jsx so they get recognized

@sassanh
Copy link

sassanh commented Sep 5, 2016

It happened again, in a vim session that was open for few hours, indentation went wrong. After restarting vim it worked alright.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 5, 2016

so what do i do with that information? the conditions absolutely prevent that from happening, my guess is jsx

@sassanh
Copy link

sassanh commented Sep 5, 2016

Yeah, I know that information is not helpful. Just though maybe it triggers something for you. I try to find out how to reproduce it.

@bounceme
Copy link
Collaborator Author

bounceme commented Sep 5, 2016

Well I have noticed jsx changing indent when repeatedly indenting

@sassanh
Copy link

sassanh commented Sep 5, 2016

Next time it happens I'll check a js file with ft=javascript. Then we can say for certain if it's related to vim-javascript at all or not. Last time it happened I was checking 3rd line of a js file (but with ft=javascript.jsx) the line it indented incorrectly wasn't in any JSXRegion. It was as simple as this:

import {x} from './y';
    import z from './w';

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