Use assets API from bat library instead of vendored code #903
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
dandavison
force-pushed
the
895-bat-assets
branch
2 times, most recently
from
c3779f7
to
a858a21
Compare
This was referenced Jan 15, 2022
dandavison
force-pushed
the
895-bat-assets
branch
2 times, most recently
from
c522e1d
to
e9f034f
Compare
dandavison
commented
Jan 15, 2022
src/options/theme.rs
Outdated
| @@ -22,16 +22,22 @@ pub fn set__is_light_mode__syntax_theme__syntax_set( | |||
| opt.syntax_theme.as_ref(), | |||
| syntax_theme_name_from_bat_theme.as_ref(), | |||
| opt.light, | |||
| &assets.theme_set, | |||
| assets.get_theme_set(), | |||
There was a problem hiding this comment.
Usage of currently private get_theme_set() here
dandavison
commented
Jan 15, 2022
src/options/theme.rs
Outdated
| Some(assets.theme_set.themes[&syntax_theme_name].clone()) | ||
| Some( | ||
| assets | ||
| .get_theme_set() |
There was a problem hiding this comment.
Usage of currently private get_theme_set() here
dandavison
commented
Jan 15, 2022
| }; | ||
| opt.computed.syntax_set = assets.syntax_set; | ||
| opt.computed.syntax_set = assets.get_syntax_set().unwrap().clone(); |
There was a problem hiding this comment.
Usage of currently private get_syntax_set() here
dandavison
commented
Jan 15, 2022
src/options/theme.rs
Outdated
| @@ -86,7 +92,7 @@ fn get_is_light_mode_and_syntax_theme_name( | |||
| theme_arg: Option<&String>, | |||
| bat_theme_env_var: Option<&String>, | |||
| light_mode_arg: bool, | |||
| theme_set: &ThemeSet, | |||
| theme_set: &LazyThemeSet, | |||
There was a problem hiding this comment.
Usage of currently private LazyThemeSet here
dandavison
commented
Jan 15, 2022
src/options/theme.rs
Outdated
| @@ -104,12 +110,12 @@ fn get_is_light_mode_and_syntax_theme_name( | |||
| // no-syntax-highlighting name. | |||
| fn valid_syntax_theme_name_or_none( | |||
| theme_name: Option<&String>, | |||
| theme_set: &ThemeSet, | |||
| theme_set: &LazyThemeSet, | |||
There was a problem hiding this comment.
Usage of currently private LazyThemeSet here
dandavison
commented
Jan 15, 2022
src/options/theme.rs
Outdated
| ) -> Option<String> { | ||
| match theme_name { | ||
| Some(name) | ||
| if is_no_syntax_highlighting_syntax_theme_name(name) | ||
| || theme_set.themes.contains_key(name) => | ||
| || theme_set.get(name).is_some() => |
There was a problem hiding this comment.
Usage of LazyThemeSet.get(theme_name) here
dandavison
commented
Jan 15, 2022
| @@ -34,7 +33,7 @@ pub fn show_syntax_themes() -> std::io::Result<()> { | |||
|
|
|||
| let make_opt = || { | |||
| let mut opt = cli::Opt::from_args(); | |||
| opt.computed.syntax_set = assets.syntax_set.clone(); | |||
| opt.computed.syntax_set = assets.get_syntax_set().unwrap().clone(); | |||
There was a problem hiding this comment.
Usage of currently private get_syntax_set() here
dandavison
commented
Jan 15, 2022
| { | ||
| writeln!( | ||
| writer, | ||
| "\n\nSyntax theme: {}\n", | ||
| title_style.paint(syntax_theme) | ||
| )?; | ||
| config.syntax_theme = Some(assets.theme_set.themes[syntax_theme.as_str()].clone()); | ||
| config.syntax_theme = Some(assets.get_theme_set().get(syntax_theme).unwrap().clone()); |
There was a problem hiding this comment.
Usage of currently private get_theme_set() here (using LazyThemeSet.get(theme_name))
dandavison
commented
Jan 15, 2022
| fn syntax_set_path() -> PathBuf { | ||
| PROJECT_DIRS.cache_dir().join("syntaxes.bin") | ||
| pub fn load_highlighting_assets() -> bat::assets::HighlightingAssets { | ||
| bat::assets::HighlightingAssets::from_cache(utils::bat::dirs::PROJECT_DIRS.cache_dir()) |
There was a problem hiding this comment.
I am still using some vendored bat code here to determine the cache directory location. If it could make sense to expose something like bin/bat/assets::assets_from_cache_or_binary() then that would be nice.
dandavison
force-pushed
the
895-bat-assets
branch
from
6c2e941
to
ddf1090
Compare
dandavison
force-pushed
the
895-bat-assets
branch
3 times, most recently
from
e315266
to
33f713a
Compare
This was referenced Feb 8, 2022
https://github.com/dandavison/delta/runs/4888306682?check_suite_focus=true Error: failed to get `bat` as a dependency of package `git-delta v0.11.3 (D:\a\delta\delta)` Caused by: failed to load source for dependency `bat` Caused by: Unable to update https://github.com/dandavison/bat?rev=0b22913108152bda7fc1890616ebfabf02c8468a#0b229131 Caused by: failed to update submodule `assets/syntaxes/Jinja2` Caused by: cannot checkout to invalid path 'Preferences/Symbol List: Blocks.tmPreferences'; class=Checkout (20) Error: The process 'C:\Users\runneradmin\.cargo\bin\cargo.exe' failed with exit code 101
dandavison
force-pushed
the
895-bat-assets
branch
from
76e67ed
to
72300d7
Compare
This reverts commit ce6814d.
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.
Proof-of-principle PR; requires exposing some additional things in bat API: dandavison/bat@0b22913
Fixes #895
Ref sharkdp/bat#2026