Add 'plist-load' feature, analogous to existing 'yaml-load' feature #345
Conversation
This file contains 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
Namely to the same file in which FontStyle is declared. This will make it easier to introduce a theme_load.rs file later.
It is safe to remove because it has never been re-exported as public API.
To make it easier to later introduce a 'plist-load' feature similar to the existing 'yaml-load' feature, but for themes and not syntaxes. I have confirmed that this change does not affect the public API.
plist brings in several dependencies, most notably the heavy xml-rs dependency.
So if you don't need to parse theme files, it is nice to be able to skip
this dependency.
For example it brings down the number of dependencies to build from 68 to 52
when building examples/synhtml.rs as a regular app. And build time shrinks
proportionally. Try yourself with:
cp examples/synhtml.rs src/main.rs # temporarily side-step dev-dependencies
cargo build --no-default-features --features parsing,html,assets,dump-load,regex-onig
keith-hall
approved these changes
Aug 11, 2021
|
Thanks, this makes sense! |
Enselic
pushed a commit
to Enselic/syntect
that referenced
this pull request
Dec 28, 2021
It is not enough to make the `default` feature depend on it. Putting code behind a feature is in practice a semver breakage. See sharkdp/bat#1991 (comment) and trishume#402. It was added in trishume#345 after doing some refactoring. We can keep the refactoring, but we choose to remove the feature for now. Another option would be to make the feature opt-out rather than opt-in and rename it to `no-plist-load` or similar. That would be too unergonomic and confusing however, so for now we just remove it. We might add it back for v5.0.0
Enselic
pushed a commit
to Enselic/syntect
that referenced
this pull request
Dec 28, 2021
It was not enough to make the `default` feature depend on it. Putting code behind a feature is in practice a semver breakage. See sharkdp/bat#1991 (comment) and trishume#402. It was added in trishume#345 after doing some refactoring. We can keep the refactoring, but we choose to remove the feature for now. Another option would be to make the feature opt-out rather than opt-in and rename it to `no-plist-load` or similar. That would be too unergonomic and confusing however, so for now we just remove it. We might add it back for v5.0.0.
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.
Why do this?
The
plistpackage thatsyntectcurrently unconditionally depends brings in several dependencies on its own, the heaviest beingxml-rs. For use cases where loading of themes is not needed, it is pleasant to not have to compile unneeded dependencies.This new
plist-loadfeature is very similar to the existingyaml-loadfeature, both in purpose, and how it is implemented. Whereasyaml-loadis for syntax definitions,plist-loadis for themes and theme preferences.Public API is not affected
I have taken care not to change the public API (with all features enabled) with this change.
That the public API has not changed (at least not in terms of added/removed/moved items) can be verified with this little oneliner:
Example of usefulness in practice
To demonstrate that this feature is useful, I recommend to compare the number of dependencies you have to build when you do this:
Before this change, you need to build 68 dependencies. After this change, it is only necessary to build 52 dependencies.
Guide to Code Review
I recommend to Code Review each commit in this PR individually.
The first two commits are simple refactorings. The third commit just cuts and pastes code around. The fourth commit introduces the
plist-loadfeature, and also adds an automated CI test for this feature.