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

More style checking. #1954

Merged
merged 3 commits into from May 9, 2015

Conversation

Projects
None yet
4 participants
@madmaxoft
Member

madmaxoft commented May 6, 2015

Spaces around some operators are checked.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft May 6, 2015

Member

Unfortunately the spaces-around-operators is quite difficult and had a few false positives, so I had to work around them. Spaces around minus cannot be checked at all, due to minus being used as a hyphen between words.

Member

madmaxoft commented May 6, 2015

Unfortunately the spaces-around-operators is quite difficult and had a few false positives, so I had to work around them. Spaces around minus cannot be checked at all, due to minus being used as a hyphen between words.

@bearbin

This comment has been minimized.

Show comment
Hide comment
@bearbin

bearbin May 6, 2015

Member

There are a huge amount of false positives for "/", so many I don't think it's really worth it to check for spaces there.

Member

bearbin commented May 6, 2015

There are a huge amount of false positives for "/", so many I don't think it's really worth it to check for spaces there.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft May 6, 2015

Member

It's not that bad, and I worked around all of them anyway.

Member

madmaxoft commented May 6, 2015

It's not that bad, and I worked around all of them anyway.

@tigerw

This comment has been minimized.

Show comment
Hide comment
@tigerw

tigerw May 7, 2015

Member

Those are quite a lot of somewhat unnecessary changes; could you get the script to ignore lines that are comments?

Member

tigerw commented May 7, 2015

Those are quite a lot of somewhat unnecessary changes; could you get the script to ignore lines that are comments?

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft May 7, 2015

Member

Oh I don't mind the changes in the comments at all, all of those are good. The only thing I don't quite like is the `" HTTP/1.0" needing to be split into two strings. If you can write a Lua pattern that matches the others but not this, be my guest.

Member

madmaxoft commented May 7, 2015

Oh I don't mind the changes in the comments at all, all of those are good. The only thing I don't quite like is the `" HTTP/1.0" needing to be split into two strings. If you can write a Lua pattern that matches the others but not this, be my guest.

@tigerw

This comment has been minimized.

Show comment
Hide comment
@tigerw

tigerw May 7, 2015

Member

Though it's still an additional (comment) style for people to conform to, and not out of necessity either, but a side-effect from something else. Since comments should follow language, not code conventions, it would be better for the style checker to ignore them. @bearbin also recommends that similarly strings be ignored also, which also fixes your problem.

Member

tigerw commented May 7, 2015

Though it's still an additional (comment) style for people to conform to, and not out of necessity either, but a side-effect from something else. Since comments should follow language, not code conventions, it would be better for the style checker to ignore them. @bearbin also recommends that similarly strings be ignored also, which also fixes your problem.

@worktycho

This comment has been minimized.

Show comment
Hide comment
@worktycho

worktycho May 7, 2015

Member

The problem with comments is that its difficult to write a pattern to check is something is in a comment. I could probably manage a perl regex to do it, but even that would be a mess. Its certainly problematic with patterns to avoid: /* a b c */ a/a /* a */ getting ignored.

Member

worktycho commented May 7, 2015

The problem with comments is that its difficult to write a pattern to check is something is in a comment. I could probably manage a perl regex to do it, but even that would be a mess. Its certainly problematic with patterns to avoid: /* a b c */ a/a /* a */ getting ignored.

@worktycho

This comment has been minimized.

Show comment
Hide comment
@worktycho

worktycho May 7, 2015

Member

The regex is: ^(\/\*([^*]|\*[^/])*\*\/[^op]*)*(([^ ]op)|op[^ *]) where op is the operator. I don't know how you'ld write that in lua patterns.

Member

worktycho commented May 7, 2015

The regex is: ^(\/\*([^*]|\*[^/])*\*\/[^op]*)*(([^ ]op)|op[^ *]) where op is the operator. I don't know how you'ld write that in lua patterns.

@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft May 9, 2015

Member

I've fixed the operator-space patterns, now they ignore everything near doublequotes. So I got rid of the unwanted changes.

Member

madmaxoft commented May 9, 2015

I've fixed the operator-space patterns, now they ignore everything near doublequotes. So I got rid of the unwanted changes.

@worktycho

This comment has been minimized.

Show comment
Hide comment
@worktycho

worktycho May 9, 2015

Member

This is being got by the broken master. Can you rebase on to the fix?

Member

worktycho commented May 9, 2015

This is being got by the broken master. Can you rebase on to the fix?

madmaxoft added a commit that referenced this pull request May 9, 2015

@madmaxoft madmaxoft merged commit ca94944 into master May 9, 2015

3 of 4 checks passed

continuous-integration/appveyor AppVeyor build cancelled
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage remained the same at 0.0%
Details
@madmaxoft

This comment has been minimized.

Show comment
Hide comment
@madmaxoft

madmaxoft May 9, 2015

Member

The Appveyor build has been stuck so I cancelled it, the other builds passed.

Member

madmaxoft commented May 9, 2015

The Appveyor build has been stuck so I cancelled it, the other builds passed.

@tigerw tigerw deleted the MoreStyleCheck branch May 9, 2015

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