You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Right now, jrnl assumes the config-file is well-formed and without issues. It processes information from the configuration as it needs it, but if the config YAML has an issue, it can cause crashes with confusing stack traces. This is a deterrent for new users, but also kind of a pain even for advanced users that frequently modify their config file.
For instance, it should be easy to check for the presence of a "journals" key and one or more journals below it. It should also be relatively easy to check that data types and valid values for certain fields are correct. For instance, encrypt can only be true or false.
I think this issue dovetails off of the #1102 to refactor configuration retrieval since there is probably an opportunity for to have a single point of truth for some validations (like data type).
I see three main passes to do in the validation process:
Can the config be parsed at all? If not, show a message that it couldn't be parsed. Include the exception that bubbles up from ruamel, since it has line and column numbers where the problem happens.
Is each value valid? Check for required values, and for whether their data types are valid. Is journals a list, is timeformat provided, is encrypt true or false, etc.
Cross-cutting validations. These would be a bit more complicate: for instance, is the path of the journal valid, or is the timeformat a valid timeformat (can such a check be done?). These might be outside of the scope of the original fix, but if so, let's spin off new issues for these.
I also think that whenever the validation fails, the error message should include:
The path the to the config file that's being used (it may be different if --config-file was supplied)
If I have a bunch of malformed text at the beginning of my config file, I get this now:
┏━ Error ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ ScannerError ┃
┃ mapping values are not allowed here ┃
┃ in "C:\Users\micah\.config\jrnl\jrnl.yaml", line 6, column 7 ┃
┃ ┃
┃ This is probably a bug. Please file an issue at: ┃
┃ https://github.com/jrnl-org/jrnl/issues/new/choose ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
But it'd be nicer if I got something like this:
┏━ Error ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Couldn't parse your configuration file at "C:\Users\micah\.config\jrnl\jrnl.yaml" ┃
┃ ┃
┃ Parse error: ┃
┃ mapping values are not allowed here ┃
┃ in "C:\Users\micah\.config\jrnl\jrnl.yaml", line 6, column 7 ┃
┃ ┃
┃ For config reference, visit: https://jrnl.sh/en/stable/reference-config-file/ ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
Wrong data type
If I put "null" in my journals key as seen in #1681, I currently get this:
┏━ Error ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ TypeError ┃
┃ argument of type 'NoneType' is not iterable ┃
┃ ┃
┃ This is probably a bug. Please file an issue at: ┃
┃ https://github.com/jrnl-org/jrnl/issues/new/choose ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
But it'd be nicer if I got something like this:
┏━ Error ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┓
┃ Couldn't parse your configuration file at "C:\Users\micah\.config\jrnl\jrnl.yaml" ┃
┃ ┃
┃ Parse error: ┃
┃ Your `journal` config key is the wrong data type. ┃
┃ It should be a list, but it is a NoneType. ┃
┃ ┃
┃ For config reference, visit: https://jrnl.sh/en/stable/reference-config-file/ ┃
┗━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━┛
Other Information
Would be worth considering how to fold these validations into --config-override as well.
It absolutely must be compatible with --config-file.
See #1645 and #1681 for support requests that would be good to make BDD tests from. The posters probably wouldn't have needed support if this enhancement existed.
Might need some discussion about what is actually "required" and we would also need to be careful not to be too stringent (for instance, paths may include environment variables, so if we're validating journal paths, we ought to ensure those don't fail validation).
If we're validating anything about a journal, we may want to confine validations to only the active journal. After all, jrnl only loads one journal at a time and a user may have a reason for including a nonexistent journal in their config file. For instance, maybe they have a secondary journal on a path to removable media, but this shouldn't prevent them from using their default journal on their primary drive.
I imagine there are other "gotchas" to think about as well. Feel free to link relevant issues or bring up other concerns.
The text was updated successfully, but these errors were encountered:
Use Case/Motivation
Right now, jrnl assumes the config-file is well-formed and without issues. It processes information from the configuration as it needs it, but if the config YAML has an issue, it can cause crashes with confusing stack traces. This is a deterrent for new users, but also kind of a pain even for advanced users that frequently modify their config file.
For instance, it should be easy to check for the presence of a "journals" key and one or more journals below it. It should also be relatively easy to check that data types and valid values for certain fields are correct. For instance,
encrypt
can only betrue
orfalse
.I think this issue dovetails off of the #1102 to refactor configuration retrieval since there is probably an opportunity for to have a single point of truth for some validations (like data type).
I see three main passes to do in the validation process:
journals
a list, istimeformat
provided, isencrypt
true or false, etc.I also think that whenever the validation fails, the error message should include:
--config-file
was supplied)Example Usage
Can't parse config file at all
If I have a bunch of malformed text at the beginning of my config file, I get this now:
But it'd be nicer if I got something like this:
Wrong data type
If I put "null" in my
journals
key as seen in #1681, I currently get this:But it'd be nicer if I got something like this:
Other Information
Would be worth considering how to fold these validations into
--config-override
as well.It absolutely must be compatible with
--config-file
.See #1645 and #1681 for support requests that would be good to make BDD tests from. The posters probably wouldn't have needed support if this enhancement existed.
Might need some discussion about what is actually "required" and we would also need to be careful not to be too stringent (for instance, paths may include environment variables, so if we're validating journal paths, we ought to ensure those don't fail validation).
If we're validating anything about a journal, we may want to confine validations to only the active journal. After all, jrnl only loads one journal at a time and a user may have a reason for including a nonexistent journal in their config file. For instance, maybe they have a secondary journal on a path to removable media, but this shouldn't prevent them from using their default journal on their primary drive.
I imagine there are other "gotchas" to think about as well. Feel free to link relevant issues or bring up other concerns.
The text was updated successfully, but these errors were encountered: