Skip to content
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

Rework syntect plugin #330

Open
CosmicHorrorDev opened this issue Jul 12, 2023 · 2 comments
Open

Rework syntect plugin #330

CosmicHorrorDev opened this issue Jul 12, 2023 · 2 comments

Comments

@CosmicHorrorDev
Copy link
Contributor

CosmicHorrorDev commented Jul 12, 2023

Had a lot of ideas floating around, so figured it would be good to open an issue that covers all of them

Existing issues / papercuts:

  1. SyntectAdapter panics when trying to use a theme name that isn't contained in the theme_set
  2. SyntectAdapter holds an entire theme set when holding the single theme being used would suffice
  3. No way to use SyntectAdapter without pulling in the embedded defaults from syntect
    • I'm currently using an external syntax definitions in one of my projects, but I still wind up with ~0.8 MiB of extra data in the final binary from syntects default syntax definitions
  4. No obvious way to know which themes are included in the default set

Spitballing a new API

I was thinking providing something along the lines of

struct SyntectAdapter {
    theme: Theme,
    syntax_set: SyntaxSet,
}

impl SyntectAdapter {
    fn defaults(default: DefaultTheme) -> Self { ... }
    fn empty() -> SyntectBuilder { ... }
}

struct SyntectBuilder {
    ...
}

impl SyntectBuilder {
    fn default_theme(mut self, default: DefaultTheme) -> Self { ... }
    fn custom_theme(mut self, theme: Theme) -> Self { ... }
    fn default_syntax_set(mut self) -> Self { ... }
    fn custom_syntax_set(mut self, syntax_set: SyntaxSet) -> Self { ... }
    fn finish(mut self) -> SyntectAdapter { ... }
}

#[non_exhaustive]
enum DefaultTheme {
    Base16OceanDark,
    Base16EightiesDark,
    Base16MochaDark,
    Base16OceanLight,
    InspiredGithub,
    SolarizedDark,
    SolarizedLight,
}

1) and 2) are addressed by moving getting the actual Theme to use to caller code, 3) is addressed by adding an extra syntect-no-defaults feature that would remove SyntectAdapter::defaults(), SyntectBuilder::default_theme(), and SyntectBuilder::default_syntax_set(), and 4) is addressed through adding DefaultTheme and SyntectBuilder::default_theme()

@CosmicHorrorDev
Copy link
Contributor Author

I think point 3) is nil since the linker seems smart enough to throw away the unused embedded asset, so I don't think that adding more features to control that makes sense. I don't think that changes my ideal API though

@lambda-fairy
Copy link
Contributor

I ran into a similar paper cut with themes. I'm using a theme that's almost the same as the default InspiredGitHub, but has a different background color. Having to override the ThemeSet for this use case makes it more troublesome than it needs to be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants