Skip to content
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

Fix 'Missing newline before ")"' when no params #364

Merged
merged 2 commits into from
Apr 2, 2019

Conversation

jlmd
Copy link
Contributor

@jlmd jlmd commented Mar 31, 2019

Fix an issue where the Exceeded max line length rule is always trying to put parameters on separate lines, even when there are no parameters, causing the rule Missing newline before ")" to break.

Fixes #327

Avoid checking "parameter-list-wrapping" rule when the element doesn't
have parameters.

Fixes pinterest#327
@shashachu
Copy link
Contributor

@jlmd just to confirm - you've tested this is still broken on 0.30.0? @shyiko addressed #327 for that release, so just want to make sure this is an update to that commit.

@jlmd
Copy link
Contributor Author

jlmd commented Apr 1, 2019

@shashachu yes, I tested it on 0.30.0 and is still broken. I'm not sure what the fix of @shyiko does, but for sure it doesn't fix #327 issue. Maybe the issue was not clear enough, that's why I created the PR.

Do you think it makes sense to revert in this PR that commit (b6f9ad5), since it seems that it doesn't really fix any issue? Even the tests that he added pass by removing the condition that he added.

@shashachu
Copy link
Contributor

Let's wait a day or so to see if @shyiko responds about what his original change was meant to do.

@shyiko
Copy link
Collaborator

shyiko commented Apr 2, 2019

@jlmd is correct (b6f9ad5 should be reverted)

@shashachu
Copy link
Contributor

Awesome thanks for confirming, @shyiko. @jlmd why don't you also revert that commit then.

@jlmd
Copy link
Contributor Author

jlmd commented Apr 2, 2019

Cool! Thanks @shashachu and @shyiko . I've reverted that commit in c24eb5a

@shashachu shashachu merged commit 3bfc666 into pinterest:master Apr 2, 2019
@shashachu
Copy link
Contributor

Thanks for the PR!

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