Skip to content

ignore irrelevant symbols when pinpointing undefined variable lints #1915

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

Merged
merged 13 commits into from
Apr 6, 2023

Conversation

MichaelChirico
Copy link
Collaborator

@MichaelChirico MichaelChirico commented Feb 25, 2023

Part of #1914

I'm not a fan of the descendant::*[ancestor::*] XPath, but I'm not sure it can be avoided.

@codecov-commenter
Copy link

codecov-commenter commented Feb 25, 2023

Codecov Report

Merging #1915 (4ec0a6a) into main (c6099d5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 4ec0a6a differs from pull request most recent head 8f1262a. Consider uploading reports for the commit 8f1262a to get more accurate results

@@           Coverage Diff           @@
##             main    #1915   +/-   ##
=======================================
  Coverage   98.95%   98.95%           
=======================================
  Files         113      113           
  Lines        4953     4966   +13     
=======================================
+ Hits         4901     4914   +13     
  Misses         52       52           
Impacted Files Coverage Δ
R/indentation_linter.R 100.00% <100.00%> (ø)
R/object_usage_linter.R 99.40% <100.00%> (+<0.01%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

descendant::SYMBOL
| descendant::SYMBOL_FUNCTION_CALL
descendant::SYMBOL[not(ancestor::expr[OP-TILDE])]
| descendant::SYMBOL_FUNCTION_CALL[not(ancestor::expr[OP-TILDE])]
| descendant::SPECIAL
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could this also appear in a formula?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think any infix operator used in a formula has to be defined elsewhere, so no. trying to thing of an example...

foo <- function() {
  lm(y ~ X %**% Z, data = X %**% Y)
}

If checkUsage is dinging %**%, it should also be dinged in the formula.

The only thing I can think of is where the operator has some special definition inside the formula, which doesn't happen in base since only %in% has special interpretation:

https://stat.ethz.ch/R-manual/R-devel/library/stats/html/formula.html

I guess it's conceivable for other downstreams to define special interpretation methods for formulas, but because checkUsage() ignores formulas entirely (IIUC), it would have to be that both (1) %**% has a special interpretation and (2) it's being used incorrectly outside of a formula.

That seems pretty out there...

@MichaelChirico MichaelChirico requested a review from AshesITR March 1, 2023 00:09
@MichaelChirico MichaelChirico changed the title ignore symbols in formula when pinpointing undefined variable lints ignore irrelevant symbols when pinpointing undefined variable lints Mar 1, 2023
@MichaelChirico
Copy link
Collaborator Author

gentle ping @AshesITR

@MichaelChirico MichaelChirico added this to the 3.1.0 milestone Mar 29, 2023
AshesITR
AshesITR previously approved these changes Apr 6, 2023
AshesITR
AshesITR previously approved these changes Apr 6, 2023
@IndrajeetPatil IndrajeetPatil merged commit fb294cc into main Apr 6, 2023
@IndrajeetPatil IndrajeetPatil deleted the obj-us-meta branch April 6, 2023 14:30
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.

4 participants