-
Notifications
You must be signed in to change notification settings - Fork 372
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
Use assets API from bat library instead of vendored code #903
Conversation
c3779f7
to
a858a21
Compare
c522e1d
to
e9f034f
Compare
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private get_theme_set()
here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private get_theme_set()
here
}; | ||
opt.computed.syntax_set = assets.syntax_set; | ||
opt.computed.syntax_set = assets.get_syntax_set().unwrap().clone(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private get_syntax_set()
here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private LazyThemeSet
here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private LazyThemeSet
here
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of LazyThemeSet.get(theme_name)
here
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private get_syntax_set()
here
{ | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Usage of currently private get_theme_set()
here (using LazyThemeSet.get(theme_name)
)
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6c2e941
to
ddf1090
Compare
e315266
to
33f713a
Compare
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
76e67ed
to
72300d7
Compare
This reverts commit ce6814d.
Proof-of-principle PR; requires exposing some additional things in bat API: dandavison/bat@0b22913
Fixes #895
Ref sharkdp/bat#2026