Skip to content

Speed up object_overwrite_linter #2344

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 3 commits into from
Nov 23, 2023
Merged

Conversation

AshesITR
Copy link
Collaborator

@AshesITR AshesITR commented Nov 23, 2023

system.time(lint_package(linters = object_overwrite_linter()))
# this
#   user  system elapsed 
#  5.744   0.010   5.752 

# main
#   user  system elapsed 
# 18.869   0.026  18.896 

While working on it, I wondered, if the lint message could be made more elegant.
WDYT about

"'fun' overwrites 'pkg::fun'. Avoid overwriting symbols from other packages."

@codecov-commenter
Copy link

codecov-commenter commented Nov 23, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1c36e0d) 99.40% compared to head (4247397) 99.40%.

❗ Current head 4247397 differs from pull request most recent head 9cc285a. Consider uploading reports for the commit 9cc285a to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #2344   +/-   ##
=======================================
  Coverage   99.40%   99.40%           
=======================================
  Files         123      123           
  Lines        5585     5586    +1     
=======================================
+ Hits         5552     5553    +1     
  Misses         33       33           

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

@IndrajeetPatil
Copy link
Collaborator

Maybe Avoid overwriting symbols from base/recommended R packages.?

"Other packages" leaves too much room for interpretation.

@AshesITR
Copy link
Collaborator Author

packages= is configurable. Maybe Avoid overwriting symbols from this package.?

@IndrajeetPatil
Copy link
Collaborator

Yes, that sounds much better to me.

@AshesITR
Copy link
Collaborator Author

@MichaelChirico WDYT? We can do this in one PR or file for follow-up.

Copy link
Collaborator

@MichaelChirico MichaelChirico left a comment

Choose a reason for hiding this comment

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

Approving now & we can continue discussion on the message in follow-up if needed (but feel free to add more commits to this PR)

is_bad <- assigned_symbols %in% pkg_exports$name
source_pkg <- pkg_exports$package[match(assigned_symbols[is_bad], pkg_exports$name)]
lint_message <- sprintf(
"'%s' is an exported object from package '%s'. Avoid re-using such symbols.",
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copying here to put it all side-by-side:

'fun' overwrites 'pkg::fun'. Avoid overwriting symbols from other packages.

I don't like 'overwrites' which is misleading. 'masks' might be better. OTOH I don't see all that much wrong with the current message. Alternative:

'%s' is exported by package '%s'. Avoid re-using function names for local symbols.

@AshesITR AshesITR merged commit cc30046 into main Nov 23, 2023
@AshesITR AshesITR deleted the refac/object_overwrite_performance branch November 23, 2023 16: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