-
Notifications
You must be signed in to change notification settings - Fork 187
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
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. |
Maybe "Other packages" leaves too much room for interpretation. |
|
Yes, that sounds much better to me. |
@MichaelChirico WDYT? We can do this in one PR or file for follow-up. |
There was a problem hiding this 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.", |
There was a problem hiding this comment.
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.
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."