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

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

Closed
dandavison opened this issue Jan 15, 2022 · 4 comments
Closed
Labels
feature-request New feature or request

Comments

@dandavison
Copy link
Contributor

Hi bat developers! Delta has, since the beginning, made use of bat's very helpful collection of language syntax and highlighting theme definitions. Until now, that's involved bat code vendored in the delta repo. I'd love to use the bat library for this. However, from a quick look, it appears that while the syntect syntax structs are exposed, the syntect themes are not (only theme name strings are exposed). Would it be possible (and fit with your aims) to provide an API to look up a syntect theme struct by theme name?

Thanks for all the work.

@dandavison dandavison added the feature-request New feature or request label Jan 15, 2022
@Enselic
Copy link
Collaborator

Enselic commented Jan 15, 2022

Hi and thanks for the request!

I think the first step here would be to replace the vendored code in the delta repo with a normal bat Cargo dependency and see how far that takes you.

I'm thinking you would leave in placeholders in the code like

// Here we would like to use any theme, not just the default
let theme = default_theme();

but otherwise get the code to compile and run. Once that is functional, we can more easily prototype what kind of API that is missing.

Does that sound reasonable?

@sharkdp
Copy link
Owner

sharkdp commented Jan 15, 2022

Would it be possible (and fit with your aims) to provide an API to look up a syntect theme struct by theme name?

Not that I disagree with the approach suggested by @Enselic, but to give a more direct answer: yes, I think we generally want to provide this functionality to enable delta (and others) to use bat-as-a-library.

Note that we currently have this notice in the documentation regarding the usage of the "expert" API (everything except for the things that are directly accessible via bat::Xyz):

If you need more control, you can also use the structs in the submodules […], but note 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.

Meaning that the API of (for example) bat::assets::HighlightingAssets might not (yet) be well designed for public usage. So in this sense, an approach as described above might be very helpful. Not just for new public-facing functionality, but also for the existing things inside submodules.

dandavison added a commit to dandavison/delta that referenced this issue Jan 15, 2022
dandavison added a commit to dandavison/bat that referenced this issue Jan 15, 2022
dandavison added a commit to dandavison/delta that referenced this issue Jan 15, 2022
@dandavison
Copy link
Contributor Author

dandavison commented Jan 15, 2022

Thanks @Enselic & @sharkdp! I've made a proof-of-principle branch in delta: dandavison/delta#903

However, it isn't correct yet and in local development I'm hitting the malloc: can't allocate region errors due -- I believe -- to the recent syntect bug. Would you be able to tell me which syntect version / revision I should be using in delta and bat, and whether I need to change my local workflows? I've run cargo clean (and rustup update).

dandavison added a commit to dandavison/delta that referenced this issue Jan 15, 2022
@dandavison
Copy link
Contributor Author

dandavison commented Jan 15, 2022

in local development I'm hitting the malloc: can't allocate region errors

Please ignore that -- this problem has gone away. (I'm not sure why it was occurring) It was because I needed to rebuild my local bat assets cache.

So,

I will add comments to the delta PR highlighting places where I have used something from bat that is not currently exposed.

Note that we currently have this notice in the documentation regarding the usage of the "expert" API

Right. I think I would still be happier responding to breaking bat expert API changes than manually updating vendored code.

dandavison added a commit to dandavison/delta that referenced this issue Jan 20, 2022
dandavison added a commit to dandavison/delta that referenced this issue Jan 22, 2022
@Enselic Enselic closed this as completed in 4e36a56 Feb 8, 2022
dandavison added a commit to dandavison/delta that referenced this issue Feb 27, 2022
dandavison added a commit to dandavison/delta that referenced this issue Feb 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-request New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants