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

Make spaces_left_parentheses_linter more robust when determining ( type #128

Merged
merged 4 commits into from
Dec 29, 2015

Conversation

saurfang
Copy link
Contributor

Currently spaces_left_parentheses_linter contains false negatives when it comes to

for(i in seq_along(1:10)) { }

where if, for, while blocks could contain function calls.

It seems to me that we can improve this linter by stricter checking on the type of ('s owner. Previously if the line has any function calls, we completely let it go.

I have also incorporated the improved linter's suggestions in this PR. Hope that is okay.

Finally I'm not very familiar with this package yet. So definitely let me know if you see any concerns or potential improvements of my PR.

# only look terminal tokens
terminal_types <- before_parsed[before_parsed$terminal, "token"]
# find the type of last such token
type <- terminal_types[length(terminal_types)]
Copy link
Member

Choose a reason for hiding this comment

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

Maybe change this to

last_type <- tail(terminal_types, n = 1)

Which might make it more explicit so a comment is unnecessary.

@saurfang
Copy link
Contributor Author

Thanks for the suggestion @jimhester. I have made corresponding changes. I have also added ']' to the whitelist per #75, which I think makes sense and is relevant to this PR improving robustness of this particular linter. Let me know if you have any other thoughts or would like me to rebase this PR.

@jimhester
Copy link
Member

Looks great, thanks!

jimhester added a commit that referenced this pull request Dec 29, 2015
Make spaces_left_parentheses_linter more robust when determining `(` type
@jimhester jimhester merged commit 6381970 into r-lib:master Dec 29, 2015
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.

2 participants