-
Notifications
You must be signed in to change notification settings - Fork 255
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
Add loco doctor --config
#736
Conversation
src/doctor.rs
Outdated
@@ -166,3 +166,17 @@ pub fn check_seaorm_cli() -> Check { | |||
}, | |||
} | |||
} | |||
|
|||
/// Prints the configuration and its path. | |||
pub fn print_config(config: &Config) -> Result<(), Error> { |
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.
This function is missing tests.
It would be better to modify it so that it doesn't return a string directly, allowing us to use the insta
crate to test the output.
Alternatively, we could implement fmt::Display
for the struct to handle printing the config details.
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.
Thank you for the feedback! I have implemented fmt::Display
for the Config
struct.
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.
can you add test?
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.
Thank you for your patience. I have added the test.
src/config.rs
Outdated
@@ -73,6 +73,9 @@ pub struct Config { | |||
/// accessing `ctx.config.settings`. | |||
#[serde(default)] | |||
pub settings: Option<serde_json::Value>, | |||
|
|||
#[serde(skip)] | |||
pub path: Option<PathBuf>, |
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 think we can remove it, and print only the environment name
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.
Thank you! I have removed the path field and updated the code to print only the environment name.
examples/demo/tests/cli_tests.rs
Outdated
@@ -1,5 +1,25 @@ | |||
#[test] | |||
fn cli_tests() { | |||
let t = trycmd::TestCases::new(); | |||
t.case("tests/cmd/*.trycmd"); | |||
t.case("tests/cmd/*.trycmd") |
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.
Upon reviewing this test, it seems overly complex and difficult to maintain. Let's go ahead and remove it—apologize for the inconvenience
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 see your point, and I’ve removed the test.
for (_, check) in doctor::run_all(&config).await { | ||
if !should_exit && !check.valid() { | ||
should_exit = true; | ||
Commands::Doctor { config: config_arg } => { |
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.
config_arg => config
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.
Thanks for the feedback. I used config_arg
to avoid a naming conflict with the outer config, which is a config::Config
struct. config_arg
is a bool
, so renaming it back to config
would cause issues when referencing the config::Config
struct. Keeping the names distinct is necessary to ensure the code functions correctly.
fixes #685