Make the grep dependency honest; drop unused requires#217
Merged
Conversation
helm-projectile reads `grep-find-ignored-files'/`-directories' but only relied on `grep' being loaded transitively by Helm. Require `grep' from the two helpers that use those vars, so they are self-sufficient if that transitive load ever goes away. Also drop the `helm-locate' and `helm-global-bindings' requires - nothing here uses a symbol from either. This is dependency hygiene, not a startup-time change: Helm's own module graph still loads all three, so the load cost is unchanged.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #217 +/- ##
==========================================
+ Coverage 46.14% 46.33% +0.19%
==========================================
Files 1 1
Lines 557 559 +2
==========================================
+ Hits 257 259 +2
Misses 300 300 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This was #2 on the list ("defer the eager requires for startup"), but the investigation changed the conclusion, so the scope is smaller and honestly framed.
Finding: the eager requires can't be deferred for a startup win. helm-projectile builds Helm sources at load (top-level
defclass/helm-make-source), so Helm is a hard load-time dependency, and Helm's own module graph transitively loadsgrep,helm-locateandhelm-global-bindingsregardless. I verified(featurep 'grep)etc. are alltright after(require 'helm-projectile)even with the explicit requires removed. Deferring would need every source definition restructured to be lazy - too big/risky for the payoff.What this does instead (dependency hygiene, no perf claim):
require 'grepfromhelm-projectile--ignored-files/-directories, the two helpers that actually readgrep-find-ignored-files/-directories, so they're self-sufficient instead of relying on a transitive load that could change. (Also finally retires the years-oldTODO: defer this require.)helm-locateandhelm-global-bindingsrequires - nothing here uses a symbol from either.Load cost is unchanged; the dependencies are just honest now. Compile (warnings-as-errors), checkdoc and 66 specs all pass.
M-x checkdocwarnings