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

Add linter lll #93

Merged
merged 1 commit into from Feb 29, 2016

Conversation

Projects
None yet
2 participants
@walle
Contributor

walle commented Feb 28, 2016

This commit adds lll (Line Length Linter) (https://github.com/walle/lll)

@alecthomas

This comment has been minimized.

Show comment
Hide comment
@alecthomas

alecthomas Feb 28, 2016

Owner

I'm not opposed to this, but there are a couple of problems:

  1. No way to change the default line length. A flag should be plumbed through in in a similar way to how --cyclo-over.
  2. It should pass -g by default.
  3. It recurses by default. Even though it's fast, with vendored source this can still take quite some time.
Owner

alecthomas commented Feb 28, 2016

I'm not opposed to this, but there are a couple of problems:

  1. No way to change the default line length. A flag should be plumbed through in in a similar way to how --cyclo-over.
  2. It should pass -g by default.
  3. It recurses by default. Even though it's fast, with vendored source this can still take quite some time.
@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Feb 28, 2016

Contributor

I've fixed number one and two, in the latest commit.
Regarding number tree, the vendor dir is skipped by default, like in dupl (https://github.com/mibk/dupl) and can be included by using the --vendor flag.

Contributor

walle commented Feb 28, 2016

I've fixed number one and two, in the latest commit.
Regarding number tree, the vendor dir is skipped by default, like in dupl (https://github.com/mibk/dupl) and can be included by using the --vendor flag.

@alecthomas

This comment has been minimized.

Show comment
Hide comment
@alecthomas

alecthomas Feb 28, 2016

Owner

Thanks!

gometalinter explicitly uses dupl ./*.go to prevent it recursing. I've also had to fork gocyclo to do the same. It's not specifically about the "vendor" directory, but more about total directory size. Data files, etc.

That said, it looks like lll can be run with wildcards, so if you can also change the line to lll -g -l {maxlinelength} ./*.go I'll merge it in.

Owner

alecthomas commented Feb 28, 2016

Thanks!

gometalinter explicitly uses dupl ./*.go to prevent it recursing. I've also had to fork gocyclo to do the same. It's not specifically about the "vendor" directory, but more about total directory size. Data files, etc.

That said, it looks like lll can be run with wildcards, so if you can also change the line to lll -g -l {maxlinelength} ./*.go I'll merge it in.

@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Feb 29, 2016

Contributor

No, it couldn't do that. But I have updated the project to support this, since it's a good feature.

Ok, I'll change this, should I squash my commits before you merge?

Contributor

walle commented Feb 29, 2016

No, it couldn't do that. But I have updated the project to support this, since it's a good feature.

Ok, I'll change this, should I squash my commits before you merge?

@alecthomas

This comment has been minimized.

Show comment
Hide comment
@alecthomas

alecthomas Feb 29, 2016

Owner

Perfect. Yes, please, squashing would be great.

Owner

alecthomas commented Feb 29, 2016

Perfect. Yes, please, squashing would be great.

@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Feb 29, 2016

Contributor

Great, squashed them into dcbd029

Contributor

walle commented Feb 29, 2016

Great, squashed them into dcbd029

alecthomas added a commit that referenced this pull request Feb 29, 2016

@alecthomas alecthomas merged commit acda826 into alecthomas:master Feb 29, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alecthomas

This comment has been minimized.

Show comment
Hide comment
@alecthomas

alecthomas Feb 29, 2016

Owner

Thanks!

Owner

alecthomas commented Feb 29, 2016

Thanks!

@alecthomas

This comment has been minimized.

Show comment
Hide comment
@alecthomas

alecthomas Feb 29, 2016

Owner

FYI I've disabled it by default, as it's very chatty at 80 characters. It can be explicitly enabled with --enable=lll

Owner

alecthomas commented Feb 29, 2016

FYI I've disabled it by default, as it's very chatty at 80 characters. It can be explicitly enabled with --enable=lll

@walle

This comment has been minimized.

Show comment
Hide comment
@walle

walle Feb 29, 2016

Contributor

Aha, ok. I saw that you added some documentation in the readme, I just realised that I had missed that, great.

Contributor

walle commented Feb 29, 2016

Aha, ok. I saw that you added some documentation in the readme, I just realised that I had missed that, great.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment