more config simplification #65
Merged
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
Use the same semantic as `git` where `-C` is used to switch working directories. Simplify the startup sequence and responsibilities between the CLI and the engine. DRY "treefmt.toml" to config::FILENAME.
Since we do a lot of path manipulation, with commands changing directories and whatnot. It's probably easier to work with only absolute paths. If we can guarantee that, the rest will become easier.
By moving the resolution to parse time, it simplifies the core of the algorithm.
basile-henry
approved these changes
Feb 22, 2021
| None => return Err(anyhow!("treefmt.toml could not be found in {} and up. Use the --init option to create one.", cwd.display())) | ||
| } | ||
| return Err(anyhow!( | ||
| "{} could not be found in {} and up. Use the --init option to create one.", |
There was a problem hiding this comment.
I'm wondering if --init shouldn't be init as it's presented as a subcommand in the help menu.
There was a problem hiding this comment.
init is our first design. But iirc, we want to avoid GitHub's CLI styling and that's why we ended up with --init
There was a problem hiding this comment.
One option is to not make it a sub-command but just a normal flag. And then deal with the command dispatch internally.
99% of the cases will be to run the formatter, which is the command we want to optimize for.
Now that all the uses of std::env::set_current_dir have been removed, we can add a bit of caching in that crate.
Rizary
reviewed
Feb 22, 2021
| None => String::new(), | ||
| }; | ||
| // Set the command to run under its working directory. | ||
| cmd_arg.current_dir(&c.work_dir); |
There was a problem hiding this comment.
Found this issue: rust-lang/rust#37868 related to Command's current_dir, thoughts?
There was a problem hiding this comment.
Luckily all of our paths are absolute so we won't be affected by that issue :)
Get the absolute path of the formatter command during config load. If it could not be resolved, warn the user and drop that formatter from the list of commands that will get evaluated.
zimbatm
force-pushed
the
config-simplifications
branch
from
ce93070
to
449d466
Compare
There is only a single value attached to it.
zimbatm
force-pushed
the
config-simplifications
branch
from
7d4cc6a
to
3bb40df
Compare
If the manifest couldn't be loaded it's probably that the file format has changed. Ignore the error and continue.
Unify how we get back mtimes from files. Ideally, all the callers to get_mtime would be part of the cache and not split out like they currently are.
Co-authored-by: Basile Henry <bjm.henry@gmail.com>
Expose even further where the cache implementation is leaking
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.
Generally move more invariants to the top to simplify the core algorithm. Any
Option type that can be removed is a good thing.