This repository was archived by the owner on Dec 15, 2022. It is now read-only.
Make snippet parsing and expansion lazy#306
Closed
Aerijo wants to merge 2 commits intoatom:masterfrom
Closed
Conversation
This was referenced Jul 14, 2020
Contributor
Author
|
Removing the exisiting laziness functionality is still something that needs to be done for this PR to be clean. But instead I've focused on reimplementing everything from the ground up. Once (if) it matures, I may revisit making changes to this package, but for now I'm focusing all my snippet efforts on the new package. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Requirements
Description of the Change
Defers parsing of snippet bodies until used for an expansion. Also defers calculation of the initial expansion text and tab stops until used.
This lazy loading is beneficial, as the majority of snippets will not be used and therefore do not need to be parsed. The parse tree is also cached on the snippet, so there is no cost for subsequent uses.
Deferring the expansion text calculation is less useful, but necessary to make way for the introduction of variables, which may change between uses (e.g., a timestamp variable). However I've included detection of variables, and if not present the expansion details are also cached.
Some redundant properties have also been removed, such as
snippetonSnippetExpansionbecause it's uses could be replaced with local variables or more specific properties of the snippet.No tests have been added (should lazy loading / caching be tested? The visible part should be the same as previous, and that's already got tests). A test for caching at the
Snippetslevel has been removed, as it is now done on theSnippetitself. There is some existing support for lazy Snippet creation, so I can probably remove that too now that creating a Snippet instance is trivial.Alternate Designs
In order to support variables, some kind of processing on use is necessary.
Benefits
Possible Drawbacks
Some properties have changed, and I don't think this package has a well defined public interface. E.g., the
settings-viewpackage accesses snippet fields directly. It doesn't appear to be affected by this change though, as thebodyTextfield is kept (though could theoretically be removed once the parse tree is made).Applicable Issues
Broken tests unrelated; see #307
Progress for #288