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

Improve error when file cannot be read #70

Merged
merged 2 commits into from
Sep 8, 2021

Conversation

fgsch
Copy link
Member

@fgsch fgsch commented Sep 3, 2021

Use the filename in the output.

Use the filename in the output.
Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

thank you @fgsch! this is an excellent improvement to our error message.

i have a single, teensy quibble about how we convert our path into a string. other than that, this looks great! ✨

lib/src/config.rs Outdated Show resolved Hide resolved
lib/src/error.rs Show resolved Hide resolved
@fgsch
Copy link
Member Author

fgsch commented Sep 7, 2021

Somewhat related, I'd like to extend this for InvalidFastlyToml error as well.
I was thinking about copying the from_str implementation into from_file and so I can access the path.
Do you have any suggestions?

@cratelyn
Copy link
Contributor

cratelyn commented Sep 7, 2021

Somewhat related, I'd like to extend this for InvalidFastlyToml error as well.
I was thinking about copying the from_str implementation into from_file and so I can access the path.
Do you have any suggestions?

for this, i might suggest providing this context elsewhere. threading a path through all of the code that isn't otherwise concerned with the file system doesn't feel quite right, so i'd imagine that we'd have better luck logging a warning near the call-site using something like...

let config = match FastlyConfig::from_file(config_path) {
    Ok(config) => config,
    Err(err) => {
        tracing::warn!(path = ?path, err = %err, "error reading configuration file");
        return Err(err);
    }
}

you can also spell this out with res.map_err(|e| { .. })?, or using something like tap, but that's the core idea. 🙂

Copy link
Contributor

@cratelyn cratelyn left a comment

Choose a reason for hiding this comment

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

this looks good to me!

@cratelyn cratelyn merged commit 68446c7 into main Sep 8, 2021
@cratelyn cratelyn deleted the fgsch/config-file_error_reporting branch September 8, 2021 12:39
@cratelyn cratelyn mentioned this pull request Sep 8, 2021
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.

2 participants