Skip to content

fix detection of non-syntactic names in object_overwrite_linter() #2347

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 8 commits into from
Nov 24, 2023
Merged

fix detection of non-syntactic names in object_overwrite_linter() #2347

merged 8 commits into from
Nov 24, 2023

Conversation

AshesITR
Copy link
Collaborator

fixes #2346

@AshesITR AshesITR mentioned this pull request Nov 23, 2023
Closed
@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (30c7d70) 99.41% compared to head (8828493) 99.41%.

❗ Current head 8828493 differs from pull request most recent head 3f8cc86. Consider uploading reports for the commit 3f8cc86 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##             main    #2347   +/-   ##
=======================================
  Coverage   99.41%   99.41%           
=======================================
  Files         124      124           
  Lines        5640     5642    +2     
=======================================
+ Hits         5607     5609    +2     
  Misses         33       33

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

MichaelChirico
MichaelChirico reviewed Nov 23, 2023
View reviewed changes
MichaelChirico
MichaelChirico reviewed Nov 24, 2023
View reviewed changes
R/object_overwrite_linter.R
@@ -76,7 +76,7 @@ object_overwrite_linter <- function(
# test that the symbol doesn't match an argument name in the function
# NB: data.table := has parse token LEFT_ASSIGN as well
xpath_assignments <- glue("
//SYMBOL[
(//SYMBOL | //STR_CONST)[
Copy link
Collaborator

@MichaelChirico MichaelChirico Nov 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

(idle musing, nothing actionable ATM)

I wonder about the performance of //STR_CONST vs. //LEFT_ASSIGN. Actually per the original issue about avoiding //expr, they appear at roughly the same frequency, so probably it's not much difference.

#1358 (comment)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, from what I remember, only //expr stood out to be exceptionally bad cost-wise.

@MichaelChirico
Copy link
Collaborator

I reproduce the link-rot issue with ess.r-project.org on my local browser, but I assume this is a transient issue. Ignoring for now.

@MichaelChirico MichaelChirico merged commit 9e5b4ac into main Nov 24, 2023
@MichaelChirico MichaelChirico deleted the fix/2346-object-overwrite-nonsyntactic branch November 24, 2023 02:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@MichaelChirico MichaelChirico MichaelChirico approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Support non-syntactic names in object_overwrite_linter()
3 participants
@AshesITR @codecov-commenter @MichaelChirico