Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Make snippet parsing and expansion lazy #306

Closed
wants to merge 2 commits into from

Conversation

Aerijo
Copy link
Contributor

@Aerijo Aerijo commented Jul 13, 2020

Requirements

  • Filling out the template is required. Any pull request that does not include enough information to be reviewed in a timely manner may be closed at the maintainers' discretion.
  • All new code requires tests to ensure against regressions

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 snippet on SnippetExpansion because 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 Snippets level has been removed, as it is now done on the Snippet itself. 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

  • Reduced memory footprint (about 3x smaller when no snippets used).
  • Gives room to support dynamic features like variables

Possible Drawbacks

Some properties have changed, and I don't think this package has a well defined public interface. E.g., the settings-view package accesses snippet fields directly. It doesn't appear to be affected by this change though, as the bodyText field is kept (though could theoretically be removed once the parse tree is made).

Applicable Issues

Broken tests unrelated; see #307

Progress for #288

@Aerijo
Copy link
Contributor Author

Aerijo commented Jul 31, 2020

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.

https://github.com/Aerijo/snippets-plus

@Aerijo Aerijo closed this Jul 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

1 participant