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

Add ControlStatementRule #39

Merged
merged 6 commits into from
May 26, 2015
Merged

Conversation

andreamazz
Copy link
Contributor

Hi @jpsim
I've added the lint rule for if, for, while and do while statements as mentioned in one of the Todos in the test suite.

return StyleViolation(type: .ControlStatement,
location: Location(file: file, offset: range.location),
severity: .Low,
reason: "if,for,while,do statements shouldn't wrap their conditionals in parentheses.")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably use the regex match for the specific keyword instead of listing them all

@jpsim
Copy link
Collaborator

jpsim commented May 26, 2015

This is really great, @andreamazz! Thanks for the PR ✨

You'll have to resolve the merge conflict before we can merge this.

Also, verifyRule() has a commentDoesntViolate parameter which verifies that triggering examples don't trigger a violation when commented out, so you should use that instead of explicitly commenting out your examples.

withSyntaxKinds: [.Keyword]).map{ return ($0, "for") }
let whileStatement = file.matchPattern("\\s{0,}(while)\\s{0,}\\(",
withSyntaxKinds: [.Keyword]).map{ return ($0, "while") }
return (ifStatement + forStatement + whileStatement).map { violation in
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we separate while from do-while statements?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, this seems fine to me.

@andreamazz
Copy link
Contributor Author

Fixed the conflict 👍

@jpsim
Copy link
Collaborator

jpsim commented May 26, 2015

Looks great, @andreamazz! Could you add an entry to CHANGELOG.md under "Enhancements"?

@andreamazz
Copy link
Contributor Author

Sure! Updated.

jpsim added a commit that referenced this pull request May 26, 2015
@jpsim jpsim merged commit abbd122 into realm:master May 26, 2015
@jpsim
Copy link
Collaborator

jpsim commented May 26, 2015

Fantastic, thanks for the hard work @andreamazz!

@jpsim
Copy link
Collaborator

jpsim commented May 28, 2015

FYI, there were a few false positives with this rule, especially when using tuples in control statements, and I've attempted to address those in #48.

@andreamazz
Copy link
Contributor Author

Right, tuples did not cross my mind 😅
Thanks 👍

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