Add syntax highlighting with syntect#201
Conversation
| // We have to compensate for the way Pager changes tty detection | ||
| let mut command_io = CommandIo::new(args)?; | ||
| if command_io.color == ColorChoice::Auto && !pre_pager_stdout { | ||
| command_io.color = ColorChoice::Never | ||
| } |
There was a problem hiding this comment.
Could/should this logic be moved into the
else {
ColorChoice::Auto
};branch of CommandIo::new
There was a problem hiding this comment.
Done, thanks. I put this code here while debugging highlighting trouble with the pager and then forgot I could consolidate once I figured it out.
| let ion_syntax = &self.syntaxes.find_syntax_by_name("ion").unwrap(); | ||
| // There's a lot to learn from sharkdp/bat the subject of automated light/dark theming, | ||
| // see src/theme.rs in: https://github.com/sharkdp/bat/pull/2896 | ||
| // Here we will hardcode something "dark" until someone complains or sends a patch | ||
| let theme = &self.assets.get_theme("Monokai Extended"); //TODO: choose theme somehow | ||
| let mut highlighter = HighlightLines::new(ion_syntax, theme); |
There was a problem hiding this comment.
Could this be moved to HighlightedStreamWriter::new and the highlighter stored for re-use?
There was a problem hiding this comment.
The highlighter parse state can't be cleared or reset that I see: https://docs.rs/syntect/latest/syntect/easy/struct.HighlightLines.html
So that would have different semantics... seems like it should be more correct but on the other hand what I have now seems to be working. The larger obstacle for me is that I don't know how to handle ownership correctly. After poking at this a bit I'd rather defer this optimization to later.
| // There's a lot to learn from sharkdp/bat the subject of automated light/dark theming, | ||
| // see src/theme.rs in: https://github.com/sharkdp/bat/pull/2896 | ||
| // Here we will hardcode something "dark" until someone complains or sends a patch | ||
| let theme = &self.assets.get_theme("Monokai Extended"); //TODO: choose theme somehow |
There was a problem hiding this comment.
How do we feel about config files? :D (eg. ~/.config/ion_cli/config.toml)
Could also be a way to point to more themes, shared modules, etc. Probably something we'd need/want eventually.
| // Consider using include_dir here, e.g. include_dir!("$CARGO_MANIFEST_DIR/assets"), | ||
| // especially if we decided to go the route of managing themes ourselves | ||
| let syntaxes: SyntaxSet = | ||
| from_uncompressed_data(include_bytes!("assets/ion.newlines.packdump")) |
There was a problem hiding this comment.
What are your thoughts around 1.1 support? Is a single ion syntax that can support TDL and eexp good enough? Or would we want separate syntax defs and switch them when appropriate?
3 participants
Description of changes:
Added syntax highlighting (via
syntect) by default for all commands which use CommandIo. Syntax highlighting is disabled when output is not a terminal, or when output is binary. When a filter does not apply or encounters an error on a single itemion jqnow does not immediately exit with error but emits an error to stderr and continues with the rest of the items in the stream (consonant withjq).Let me know if printing an error or aborting the command seems more reasonable when someone specified
--color --format binary.Still TODO: light/dark detection.
ion jqHere we demonstrate syntax highlighting by default, automatic disabling of syntax highlighting when stdout is not a terminal, and the added feature of emitting an error to stderr when a filter does not apply to an individual element.
ion catHighlighting and terminal detection demonstrated with
ion cat, as well as a demonstrating detection override via flags.ion inspectinspectrequired a little customization since it uses a pager by default and that messes with terminal detection.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.