kivikakk / comrak

CommonMark + GFM compatible Markdown parser and renderer
Other
1.16k stars 141 forks source link

Rework syntect plugin #330

Open CosmicHorrorDev opened 1 year ago

CosmicHorrorDev commented 1 year ago

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
    • Really I'm talking about trishume/syntect#478

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 commented 1 year ago

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 commented 3 weeks ago

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.