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

feat: readable toml parsing error #854

Merged
merged 2 commits into from
Mar 31, 2024
Merged

feat: readable toml parsing error #854

merged 2 commits into from
Mar 31, 2024

Conversation

SoloJacobs
Copy link
Contributor

@SoloJacobs SoloJacobs commented Mar 31, 2024

See commit message for the new error message.
For more information see issue GH-847. Closes #847

Because this is my first pull request in yazi, I've tried to keep it as minimal as possible. Here are some things I noticed while working on this issue:

  • Only one file is parsed at a time, this means the user does not get information about keymap.toml, if yazi.toml has an error.
  • No unit test was written for the new functionality. I would need some pointers, if this is needed.
  • If yazi.toml is valid, but contains an unknown key, then the key appears to silently omitted.
    [manager]
    sortt_by = "natural"
    
  • If yazi.toml could not be read, due to an IO error, then the file is silently omitted.
  • It looks like there is some slight shotgun parsing happening in Preset (explanation of shotgun parsing: https://lexi-lambda.github.io/blog/2019/11/05/parse-don-t-validate/). This may be a personal preference of mine, but I would have checked that yazi.toml matches the format specified the by docs before merging it with the default values.

Previously, the yazi displayed the following error.
```
Backtrace omitted. Run with RUST_BACKTRACE=1 to display it.
Run with RUST_BACKTRACE=full to include source snippets.

The application panicked (crashed).
  called `Result::unwrap()` on an `Err` value: Error { inner: Error { inner: TomlError { message: "invalid key", raw: Some("{ mime = \"application/octet-stream\",\nuse = [ \"extract_zlib\", \"reveal\" ] }\n"), keys: [], span: Some(0..1) } } }
in yazi-config/src/preset.rs, line 42
thread: main
```

It now displays the following message instead:

```
Error: Loading "/home/solo/.config/yazi/yazi.toml"

Caused by:
    TOML parse error at line 1, column 1
      |
    1 | { mime = "application/octet-stream",
      | ^
    invalid key
```

For more information see issue sxyaziGH-847.
@SoloJacobs SoloJacobs marked this pull request as ready for review March 31, 2024 11:41
@sxyazi
Copy link
Owner

sxyazi commented Mar 31, 2024

Thank you for the PR, it's very neat.

Regarding shotgun parsing, yes, currently, the user's configuration is first merged with the default configuration without any checks; however, the parsing is done based on the merged new configuration (user configuration + default configuration) with checks.

Are you meaning doing the second step's checks in the first step as well? Or moving the checks of the second step to the first step?

@sxyazi sxyazi merged commit fd455a1 into sxyazi:main Mar 31, 2024
5 checks passed
@SoloJacobs
Copy link
Contributor Author

Ha, you are fast.

I would try to move the checks in the second step to the first step. If you are open to a refactoring commit like that, I will give it a shot. However, it might turn out that such a refactoring improves the code only by little.

@sxyazi
Copy link
Owner

sxyazi commented Mar 31, 2024

Sure, I'm open to it. I'd be happy to accept a refactoring PR if it can make the code tidier and won’t be much performance impact ;)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 1, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

More readable config errors
2 participants