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

More explicit config error message #1240

Merged
merged 5 commits into from
Jul 29, 2021

Conversation

hu55a1n1
Copy link
Member

@hu55a1n1 hu55a1n1 commented Jul 27, 2021

Closes: #1021

Description

Modifies the configuration file parse error message, so an error that previously looked like so ->

$ hermes start
error: hermes fatal error: parse error: unknown variant `naive`, expected `packets` or `all` for key `global.strategy` at line 62 column 1

LE @adizere -- Now looks like ->

$ hermes start
The Hermes configuration file at path '/Users/adi/.hermes/config.toml' is invalid, reason: parse error: unknown variant `naive`, expected `packets` or `all` for key `global.strategy` at line 49 column 1
Please see the example configuration for detailed information about the supported configuration options: https://github.com/informalsystems/ibc-rs/blob/master/config.toml

For contributor use:

  • Added a changelog entry, using unclog.
  • If applicable: Unit tests written, added test to CI.
  • Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
  • Updated relevant documentation (docs/) and code comments.
  • Re-reviewed Files changed in the Github PR explorer.

@hu55a1n1
Copy link
Member Author

@adizere and @romac - do you think this fix is sufficient or should we also print the help output in such cases?

@romac
Copy link
Member

romac commented Jul 27, 2021

Can we instead catch the config error and print a more informative message? Maybe something like:

the Hermes configuration file is invalid, reason: etc.
Please see the example configuration for detailed information about the supported configuration options: [link to config.toml]

@hu55a1n1
Copy link
Member Author

Can we instead catch the config error and print a more informative message?

Good point! Done! 👍

@romac
Copy link
Member

romac commented Jul 28, 2021

Thanks! One last thing, can you please update the PR description with the new error message? LGTM otherwise :)

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Just had a question, otherwise looks good!

Please consider updating the changelog also (we now use unclog).

relayer-cli/src/application.rs Outdated Show resolved Hide resolved
Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Looks great, thanks Shoaib! I did one minor aesthetic change here d17d3e5. Approving and will let you do the merge if you agree with the changes I made.

@hu55a1n1 hu55a1n1 merged commit b30d763 into master Jul 29, 2021
@hu55a1n1 hu55a1n1 deleted the hu55a1n1/1021-indicate-config-file-error branch July 29, 2021 15:49
hu55a1n1 added a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
* Be more explicit about config errors

* Improve config error message

* Update changelog

* Changelog & added cfg path in the error message

* Clippy fixes after rust update to rustc 1.54

Co-authored-by: Adi Seredinschi <adi@informal.systems>
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.

indicate config file error when applicable
3 participants