Skip to content

Speed up object_overwrite_linter#2344

Merged
AshesITR merged 3 commits into
mainfrom
refac/object_overwrite_performance
Nov 23, 2023
Merged

Speed up object_overwrite_linter#2344
AshesITR merged 3 commits into
mainfrom
refac/object_overwrite_performance

Conversation

@AshesITR
Copy link
Copy Markdown
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
Copy Markdown

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
Copy Markdown
Collaborator

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

"Other packages" leaves too much room for interpretation.

@AshesITR
Copy link
Copy Markdown
Collaborator Author

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

@IndrajeetPatil
Copy link
Copy Markdown
Collaborator

Yes, that sounds much better to me.

@AshesITR
Copy link
Copy Markdown
Collaborator Author

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

Copy link
Copy Markdown
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
Copy Markdown
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