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

fix exit codes of binaries #172

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix exit codes of binaries #172

wants to merge 2 commits into from

Conversation

marcoow
Copy link
Member

@marcoow marcoow commented Dec 31, 2024

This fixes the exit codes of the project binaries that currently swallow any errors (they log the errors but always return a successful exit code. That's not critical when working with the binaries in development but leads to bugs like #169 going unnoticed in CI…

The code is quite repetitive now and could use some cleanup eventually.

This also fixes 2 actual errors in the generators which fix CI (which now correctly fails because of these errors).

closes #169

@marcoow marcoow requested a review from hdoordt December 31, 2024 11:03
@marcoow marcoow force-pushed the fix-exit-codes branch 3 times, most recently from 832e894 to 466f8bf Compare December 31, 2024 11:14
Comment on lines +130 to +141
match reset(&mut ui, &config.database).await {
Ok(db_name) => {
ui.outdent();
ui.success(&format!("Reset database {} successfully.", db_name));
Ok(())
}
Err(e) => {
ui.outdent();
ui.error("Could not reset database!", &e);
Err(e)
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not super fond of the pattern here, where we log the error manually in every single place.
I think it'd be more robust to do this logging in the main function above, before exiting with a non-zero code.

I imagine this was originally done to control the message shown to the user. We can achieve the same goal with the following structure:

Suggested change
match reset(&mut ui, &config.database).await {
Ok(db_name) => {
ui.outdent();
ui.success(&format!("Reset database {} successfully.", db_name));
Ok(())
}
Err(e) => {
ui.outdent();
ui.error("Could not reset database!", &e);
Err(e)
}
}
let outcome = reset(&mut ui, &config.database).await.context("Could not reset the database!");
ui.outdent();
let db_name = outcome?;
ui.success(&format!("Reset database {} successfully.", db_name));
Ok(())

and in main:

 match cli().await {
        Ok(_) => ExitCode::SUCCESS,
        Err(e) => {
            ui.error(e.to_string(), &e);
            ExitCode::FAILURE,
        }
    }

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ideally ui.error would just take the error, relying on its Display implementation, rather than forcing the caller to invoke e.to_string(), but that can be fixed later.

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.

Failed to render Liquid template on CRUD controller
2 participants