Enhance air format
#86
Conversation
LSP needs `format_node()` since it holds onto the AST, so this isn't as useful yet over there
| biome_formatter = { workspace = true } | ||
| clap = { workspace = true } | ||
| biome_parser = { workspace = true } | ||
| clap = { workspace = true, features = ["wrap_help"] } |
There was a problem hiding this comment.
The magic wrap_help here enables auto wrapping of help text in the cli
astral-sh/ruff#9633
| Format(FormatCommand), | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Parser)] | ||
| pub(crate) struct LspCommand {} | ||
|
|
||
| #[derive(Clone, Debug, Parser)] | ||
| #[command(arg_required_else_help(true))] |
There was a problem hiding this comment.
I really don't like "accidental" formatting since formatting is destructive. It needs to be intentional from the user. So with this air format will display the help page for format. Use air format . to format the current directory.
I think we should use this for all subcommands tbh
| #[arg(long)] | ||
| pub check: bool, |
There was a problem hiding this comment.
arg(long) tells clap to allow --check but not -c for this option (that would be arg(short))
crates/air/src/commands/format.rs
Outdated
| // TODO: Parallel directory visitor | ||
| let mut builder = ignore::WalkBuilder::new(first_path); | ||
|
|
||
| let parser_options = RParserOptions::default(); | ||
| let parsed = air_r_parser::parse(contents.as_str(), parser_options); | ||
| for path in paths { | ||
| builder.add(path); | ||
| } | ||
|
|
||
| if parsed.has_errors() { | ||
| return Ok(ExitStatus::Error); | ||
| let mut out = Vec::new(); | ||
|
|
||
| for path in builder.build() { | ||
| match path { |
There was a problem hiding this comment.
We utilize BurntSushi's ignore crate (builds on walkdir) to recursively walk the supplied paths (files or directories) and collect all nested R files
We will eventually add exclude and include criteria on top of this
We can also eventually make this a parallel walker for performance, but that is harder and can be its own thing
crates/air/src/commands/format.rs
Outdated
| let source = line_ending::normalize(source); | ||
| let formatted = match format_source(source.as_str(), options) { | ||
| Ok(formatted) => formatted, | ||
| Err(err) => return Err(FormatCommandError::Format(path.clone(), err)), | ||
| }; | ||
|
|
||
| // TODO: We rarely ever take advantage of this optimization on Windows right | ||
| // now. We always normalize on entry but we apply the requested line ending | ||
| // on exit (so on Windows we often infer CRLF on entry and normalize to | ||
| // LF, but apply CRLF on exit so `source` and `new` always have different | ||
| // line endings). We probably need to compare pre-normalized against | ||
| // post-formatted output? | ||
| match formatted { | ||
| FormattedSource::Formatted(new) => { | ||
| match mode { | ||
| FormatMode::Write => { | ||
| std::fs::write(&path, new) | ||
| .map_err(|err| FormatCommandError::Write(path.clone(), err))?; | ||
| } | ||
| FormatMode::Check => {} | ||
| } | ||
| Ok(FormatFileResult::Formatted(path)) | ||
| } | ||
| FormattedSource::Unchanged => Ok(FormatFileResult::Unchanged), | ||
| } | ||
| } |
There was a problem hiding this comment.
format_source() takes &str and parses and formats it. It returns FormattedSource which nicely tells you if the source is Unchanged or not. This is super nice as it lets you optimize by avoiding writing back to file when nothing changed. I think the LSP should also use something like this but I had trouble thinking about exactly what to generalize from here for use in the LSP, since the LSP already has its own copy of the CST that it would prefer to pass in.
There was a problem hiding this comment.
We do have an issue with our string normalization on Windows that prevents this from working well.
- Windows file contains CRLF
- We normalize to LF and store as
source, but storeLineEnding::CRLFfor the formatter to use - Formatter emits
formattedwith CRLF endings sourceandformattednow always look different
I think we should explore not doing the normalization (neither biome nor ruff do it) as I think our tooling can totally handle it, and it would open the door for better optimizations
| for error in &errors { | ||
| // TODO: Hook up a tracing subscriber! | ||
| tracing::error!("{error}"); | ||
| } |
There was a problem hiding this comment.
Errors are collected along the way for each path and are emitted at top level here. It's silent right now, but once we hook up a subscriber it will look quite nice as it reports the path and the problem for each error
For the "something has gone wrong" unexpected error case
Can now perform all of these actions to format a file or set of files:
Can now perform a
--checkwhich exits with1without writing anything if any files change