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 an exception for box::use() to infix spaces linter #1105

Merged
merged 5 commits into from
Apr 27, 2022

Conversation

klmr
Copy link
Contributor

@klmr klmr commented Apr 26, 2022

As discussed in #1087, this is a draft to extend the infix_spaces_linter with an exception for lhs/rhs expressions inside a box::use() call, which represent import declarations that should resemble file paths, and thus don’t call for spaces around /.

My XPath-fu is fairly rusty so I’d appreciate a review of the implementation. I’ve opted not to modify the rule’s current XPath, instead filtering the resulting lint list by excluding all occurrences of / inside box::use(). The XML structure of the parsed expressions makes this somewhat awkward, because rather than being nested (as I naïvely expected), the surrounding box::use() call expression is adjacent to the parent node of the / expression.

I’m not 100% sure whether my way of testing for a box::use() call is maximally specific or whether it could lead to false positives … however, I can’t see how this would happen.

R/infix_spaces_linter.R Outdated Show resolved Hide resolved
@AshesITR
Copy link
Collaborator

Thanks for submitting this PR!

@klmr
Copy link
Contributor Author

klmr commented Apr 26, 2022

Thanks, I’ve made adjustments in response to your two comments. 👍

Copy link
Collaborator

@AshesITR AshesITR left a comment

Choose a reason for hiding this comment

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

LGTM, thanks again!

@AshesITR AshesITR linked an issue Apr 26, 2022 that may be closed by this pull request
@MichaelChirico MichaelChirico merged commit 56403b3 into r-lib:master Apr 27, 2022
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.

Support Box
3 participants