-
-
Notifications
You must be signed in to change notification settings - Fork 192
refactor: use log::Level's deserialiser, and link example TOML #1907
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
Conversation
use the serde functionality of the log crate. this will make the error
message print the accepted strings if an incorrect string is provided.
unknown variant `asd`, expected one of `ERROR`, `WARN`, `INFO`, `DEBUG`, `TRACE`
however, if you use a wrong type (like bool), it will just say
"wanted string or table".
to try and direct to docs, we link to the example toml in github.
alternatively, this could link to https://lychee.cli.rs/guides/config/
instead?
the table format from log::Level is a bit mysterious and could be
a source for confusion, but idk how to prevent that - can you somehow
assert that what is being deserialiesd is a string?
related to https://www.github.com/lycheeverse/lychee/issues/1903
|
Thank you very much for this PR. Making use of the
I've played around with deserialisation and figured that your PR is ideal. I didn't find an easy way to add more context to the error message. (anyhow cannot be used because of the return types, the only possibility would probably be to implement [verbose.DEBUG]So I think the error message is perfectly accurate. There is just the limitation that the valid variants are only listed as soon as the toml file is valid in terms of types. (verbose is string or table) Trying to list the variants before this step wouldn't make too much sense IMO. Linking in the help message to GitHub is great and I also prefer this over lychee.cli.rs, as our docs are not versioned yet. |
|
Yeah it is a bit hard to make the error messages show what you want. The other idea I was thinking about was that you deserialise a string, then use FromStr of log::Level. You'd have to map the errors and manually list the acceptable values - I'd wanted to avoid writing the list manually, but maybe it's not a big deal. I'm also not sure about whether we want to broaden the accepted input types, it was really a side-effect here. I don't know if the table format is something that we'd want to accept and continue accepting in the future. |
show the list of acceptable values even when an invalid type value is given. TODO: should we implement TryFrom or FromStr for our Verbosity type? then, we would just call that from Deserialize. however, we'd need to choose an error type and there's nothing in ErrorKind that's suitable
anyway, looks like this now
```
[ERROR] Error while loading config: Cannot load default configuration file `lychee.toml`: Failed to parse configuration file
Caused by:
TOML parse error at line 5, column 11
|
5 | verbose = {}
| ^^
invalid verbosity value, expected one of "error", "warn", "info", "debug", or "trace"
See: https://github.com/lycheeverse/lychee/blob/lychee-v0.21.0/lychee.example.toml
```
|
Hmm, I must admit that I preferred the version at 0514883. It's less code, we don't have to manually spell out the variants and construct the error ourself. I think the error message was already great with that version. Also I don't see any problems with allowing TOML table format. Or do you think the error message wasn't good enough in 0514883? |
|
Yeah, I think that the message when you put a wrong type is not clear enough if it only says "wanted string or table". From the message alone, it's not obvious that if you put a wrong string, it will tell you which strings are valid. The "or table" raises even more questions - what is the table's format and why can verbosity be a table? About the table format, does it have any benefits over a string? Does it let a user have more detailed customisation, e.g., enable debug but disable info? I just don't know, and unless we want to document and maintain this format, I don't think it should be added. I guess I'm not strictly against having table format, but I definitely think the initial error message should mention all the valid strings. Once this is in place, it could (additionally) mention "or table" with a docs reference to the table format. |
thomas-zahner
left a comment
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.
Okay sure, we can also do it like this. Should be more user-friendly at any rate.
use the serde functionality of the log crate. this will make the error
message print the accepted strings if an incorrect string is provided.
however, if you use a wrong type (like bool), it will just say
"wanted string or table".
to try and direct to docs, we link to the example toml in github.
alternatively, this could link to https://lychee.cli.rs/guides/config/
instead?
the table format from log::Level is a bit mysterious and could be
a source for confusion, but idk how to prevent mentioning - can you somehow
assert that what is being deserialised is a string?
related to #1903