Skip to content
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

Don't ignore --config-file when using --stdin #231

Merged
merged 2 commits into from
Apr 9, 2023

Conversation

lbodor
Copy link
Contributor

@lbodor lbodor commented Apr 9, 2023

Fix #230.

@@ -21,18 +22,6 @@ pub fn format_stdin_cmd(
}
};

// Search for the treefmt.toml from there.
let config_file = match config::lookup(work_dir) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't config::lookup() already happen in cli_from_args() for both format_cmd() and format_stdin_cmd()?

config::FILENAME,
work_dir.display()
));
}
Copy link
Contributor Author

@lbodor lbodor Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is already in run_cli() and now applies to both format_cmd() and format_stdin_cmd().

config::FILENAME,
cli.work_dir.display(),
));
match &cli.config_file {
Copy link
Contributor Author

@lbodor lbodor Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let me know if here you prefer is_none() followed by expect(), and I'll do that instead.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both are fine by me

Copy link
Member

@zimbatm zimbatm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks! LGTM

borg merge

// Default the tree root to the folder that contains the config file
let tree_root = tree_root.clone().unwrap_or_else(|| {
// unwrap: since the config_file is a file, there MUST be a parent folder.
config_file.clone().parent().unwrap().to_path_buf()
config_file.parent().unwrap().to_path_buf()
Copy link
Contributor Author

@lbodor lbodor Apr 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now same as

config_file.parent().unwrap().to_path_buf()

and resolves

https://github.com/numtide/treefmt/actions/runs/4649699136/jobs/8228370543

error: using `clone` on a double-reference; this will copy the reference of type `&std::path::Path` instead of cloning the inner type
  --> src/command/format_stdin.rs:28:9
   |
28 |         config_file.clone().parent().unwrap().to_path_buf()

@zimbatm
Copy link
Member

zimbatm commented Apr 9, 2023

bors merge

@bors bors bot merged commit 6466798 into numtide:main Apr 9, 2023
@lbodor
Copy link
Contributor Author

lbodor commented Apr 9, 2023

Thanks very much 👌

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

--config-file is ignored when using --stdin
2 participants