Expose syntect theme and syntax sets #2030

dandavison · 2022-01-15T15:46:55Z

Proof-of-principle draft PR allowing delta to replace vendored bat code with bat library usage.

See

#2026
dandavison/delta#903 (companion delta PR)

Ref sharkdp#2026 Ref dandavison/delta#895

Enselic · 2022-01-22T11:05:59Z

src/assets.rs

@@ -15,7 +15,7 @@ use crate::syntax_mapping::ignored_suffixes::IgnoredSuffixes;
 use crate::syntax_mapping::MappingTarget;
 use crate::{bat_warning, SyntaxMapping};

-use lazy_theme_set::LazyThemeSet;
+pub use lazy_theme_set::LazyThemeSet;


Spontaneously I would like to have a public API that hides wether or not themes are lazy-loaded. So exporting the name LazyThemeSet should be avoided if possible.

I'm a bit short on time, so maybe you could experiment a bit and see if that can be achieved?

dandavison · 2022-01-22T16:01:11Z

exporting the name LazyThemeSet should be avoided if possible.

Hi @Enselic, thanks for looking. How about something like 9c2b737? That adds a new public enum to bat named ThemeSet that currently has a single variant only (the LazyThemeSet). The new enum implements the same API as LazyThemeSet.

dandavison · 2022-01-22T16:20:23Z

src/assets/theme_set.rs

+    }
+
+    /// An iterator over all theme names
+    pub fn themes(&self) -> impl Iterator<Item = &str> {


I think perhaps this method would be better named theme_names

EDIT I made this change and force-pushed

dandavison · 2022-01-22T16:49:44Z

Since the enum has one variant only currently, I've switched to use a newtype struct in 3d01991. I think that's preferable, but we can go back to the enum in 9c2b737 if not.

dandavison · 2022-02-05T23:24:05Z

Hi @Enselic, if you do get time to look at this again that would be great but no worries there are lots of other issues to attend to currently. It will be nice for delta if it turns out to be possible to go in this direction because I'll be able to delete some vendored code and update highlighting assets simply by bumping the bat library version.

Enselic · 2022-02-06T05:43:53Z

@dandavison Sorry for letting you wait on this. I've had my plate full lately.

I haven't had time to investigate this myself yet, so maybe I should just go ahead and ask instead:

I don't fully understand why you can't use the existing API based around theme names, i.e. Config::theme. Would be great if you could elaborate on that.

dandavison · 2022-02-06T06:18:03Z

Hi @Enselic, no worries!

I don't fully understand why you can't use the existing API based around theme names, i.e. Config::theme. Would be great if you could elaborate on that.

Sure, that's easy to explain. My motivation here is to use the bat library in delta. Delta is, like bat, a Rust application that uses syntect for highlighting. So I need the bat library to give me the syntect theme structs themselves, not just the names.

The connection between delta and bat is:

They both use syntect
Delta uses bat's collection of binary syntect highlighting assets (and also copies bat's pager-spawning code).

The purpose of this PR is to alter the bat library so that a client such as delta can use the bat library for its collection of binary highlighting assets.

Enselic · 2022-02-06T16:33:39Z

Thanks for the explanation! That makes sense. If that is the case, isn't it enough to simply change

pub(crate) fn get_theme(&self, theme: &str) -> &Theme {

to

pub fn get_theme(&self, theme: &str) -> &Theme {

?

That would enable you to get the syntect Theme for a given theme name.

This reverts commit 3d01991.

This reverts commit 9c2b737.

This reverts commit 0b22913.

dandavison · 2022-02-07T04:27:48Z

Yes, thank you @Enselic! You're right -- I think because delta had always used syntect's ThemeSet, I had overlooked the possibility of using bat's get_theme and was making this change more complicated than it needed to be. Updated this PR and the companion delta PR.

sharkdp · 2022-02-07T07:19:04Z

I'm in favor of this (now minimal) change. Especially since we warn in the documentation that the API of these internal modules is much more likely to change. Some or all of these modules might be removed in the future.

Enselic · 2022-02-07T20:24:53Z

I am also fine with this change as long as the API is only considered semi-stable, but I agree the disclaimer in the docs makes that clear.

I think it would be good to repeat that disclaimer in the docs of the individual changed methods just to make sure people see them though. But it is not a super strong wish.

@dandavison Can you also add an entry in CHANGELOG.md in the "bat as a library" section for this please? More info in https://github.com/sharkdp/bat/blob/master/CONTRIBUTING.md. Thanks!

Apart from the above I consider this PR Approved 🎉

As a side-note, I have played around with the thought of having a bat crate and a bat-impl crate, similarly to the thiserror crate that depends on a thiserror-impl crate.

The bat crate would contain the PrettyPrinter stable API and the bat binary, and PrettyPrinter and the bat binary would depend on bat-impl. The API this PR adds would live in bat-impl. So delta would depend on bat-impl and not bat. Just an idea. Not sure if it is a good idea. Certainly out of scope for this PR.

dandavison · 2022-02-07T20:39:07Z

Thanks @sharkdp and @Enselic, I've add the Changelog entry.

As a side-note, I have played around with the thought of having a bat crate and a bat-impl crate,

What do you think about having a crate dedicated specifically to the collection of highlighting assets and the wrapping API for working with them? It's a very useful collection. (Are there syntect apps other than delta using them?) As a side effect I'm guessing you might appreciate not having all those submodules in your main repo.

sharkdp · 2022-02-07T20:48:23Z

What do you think about having a crate dedicated specifically to the collection of highlighting assets and the wrapping API for working with them?

This has been requested a few times, and I agree that this would certainly be very useful. I have written down my thoughts on this here: #919

Enselic · 2022-02-08T07:01:06Z

@dandavison We are currently experimenting with time based releases, and if everything goes as planned, the next release will be in April. Just to give you a sense of when the API will be available to delta.

Enselic · 2022-02-24T16:19:29Z

@dandavison @michaelblyons Heads-up that we will make a bat v0.19.1 release with this new API in the coming days.

We want to keep projects that depend on bat happy. Waiting until April to make a release seems way too long in this case. :)

dandavison · 2022-02-24T17:39:15Z

Excellent, thanks a lot @Enselic. I'll make a delta release shortly afterwards.

Enselic · 2022-02-27T07:46:15Z

Released in https://crates.io/crates/bat/0.20.0 🎉

(Turned out we had a semver breaking change, so we had to do v0.20.0 rather than v0.19.1)

Expose syntect SyntaxSet and LazyThemeSet

0b22913

Ref sharkdp#2026 Ref dandavison/delta#895

dandavison force-pushed the expose-syntect-theme-and-syntax-sets branch from cd96e16 to 0b22913 Compare January 15, 2022 16:24

dandavison mentioned this pull request Jan 15, 2022

Obtain syntect Theme structs from theme name when using bat as a library #2026

Closed

Enselic reviewed Jan 22, 2022

View reviewed changes

dandavison commented Jan 22, 2022

View reviewed changes

Do not expose LazyThemeSet publicly

9c2b737

dandavison force-pushed the expose-syntect-theme-and-syntax-sets branch from 2de7e6e to 9c2b737 Compare January 22, 2022 16:26

Use new type pattern instead of enum with one variant

3d01991

dandavison added 4 commits February 6, 2022 22:59

Revert "Use new type pattern instead of enum with one variant"

fccdada

This reverts commit 3d01991.

Revert "Do not expose LazyThemeSet publicly"

8e8f111

This reverts commit 9c2b737.

Revert "Expose syntect SyntaxSet and LazyThemeSet"

9b8089c

This reverts commit 0b22913.

Expose get_syntax_set and get_theme

ae6773e

Document public functions

85d8cc8

Enselic approved these changes Feb 7, 2022

View reviewed changes

Add changelog entry

ddc15a6

sharkdp approved these changes Feb 7, 2022

View reviewed changes

Enselic marked this pull request as ready for review February 8, 2022 06:58

Enselic merged commit 4e36a56 into sharkdp:master Feb 8, 2022

This was referenced Feb 8, 2022

🐛 Not picking custom bat theme dandavison/delta#953

Closed

🐛 Incompatible with binary caches from latest bat dandavison/delta#895

Closed

dandavison mentioned this pull request Feb 25, 2022

🚀 Update syntax definitions from bat dandavison/delta#988

Closed

epage mentioned this pull request Oct 7, 2022

Share sublime syntaxes #919

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose syntect theme and syntax sets #2030

Expose syntect theme and syntax sets #2030

dandavison commented Jan 15, 2022

Enselic Jan 22, 2022

dandavison commented Jan 22, 2022 •

edited

Loading

dandavison Jan 22, 2022 •

edited

Loading

dandavison commented Jan 22, 2022

dandavison commented Feb 5, 2022

Enselic commented Feb 6, 2022 •

edited

Loading

dandavison commented Feb 6, 2022

Enselic commented Feb 6, 2022

dandavison commented Feb 7, 2022

sharkdp commented Feb 7, 2022

Enselic commented Feb 7, 2022

dandavison commented Feb 7, 2022

sharkdp commented Feb 7, 2022

Enselic commented Feb 8, 2022

Enselic commented Feb 24, 2022

dandavison commented Feb 24, 2022

Enselic commented Feb 27, 2022 •

edited

Loading

Expose syntect theme and syntax sets #2030

Expose syntect theme and syntax sets #2030

Conversation

dandavison commented Jan 15, 2022

Enselic Jan 22, 2022

Choose a reason for hiding this comment

dandavison commented Jan 22, 2022 • edited Loading

dandavison Jan 22, 2022 • edited Loading

Choose a reason for hiding this comment

dandavison commented Jan 22, 2022

dandavison commented Feb 5, 2022

Enselic commented Feb 6, 2022 • edited Loading

dandavison commented Feb 6, 2022

Enselic commented Feb 6, 2022

dandavison commented Feb 7, 2022

sharkdp commented Feb 7, 2022

Enselic commented Feb 7, 2022

dandavison commented Feb 7, 2022

sharkdp commented Feb 7, 2022

Enselic commented Feb 8, 2022

Enselic commented Feb 24, 2022

dandavison commented Feb 24, 2022

Enselic commented Feb 27, 2022 • edited Loading

dandavison commented Jan 22, 2022 •

edited

Loading

dandavison Jan 22, 2022 •

edited

Loading

Enselic commented Feb 6, 2022 •

edited

Loading

Enselic commented Feb 27, 2022 •

edited

Loading