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

Display problems with config file when starting jrnl #1682

Open
6 tasks
micahellison opened this issue Feb 17, 2023 · 0 comments
Open
6 tasks

Display problems with config file when starting jrnl #1682

micahellison opened this issue Feb 17, 2023 · 0 comments
Labels
enhancement New feature or request

Comments

@micahellison
Copy link
Member

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 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:

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:

┏━ 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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants