✨feat: `.selene.toml` and `selene.toml` are now detected by order of priority #591

Zeioth · 2024-03-10T03:20:54Z

This PR:

add 20 lines of code, but it keep all the original checkings.
I've tried it to look as similar to the original code as possible.
It should be obvious enough.

The feature has been tested on neovim + none-ls with:

No file.
.selene.toml
selene.toml
.selene.toml + wrong syntax
selene.toml + wrong syntax

Example of bad syntax

Example of file detected OK

Example of no config file

…priority.

Zeioth · 2024-03-10T03:21:36Z

Also it closes #469

chriscerie

Need changelog + docs.

selene/src/main.rs

Zeioth · 2024-03-11T13:51:52Z

@chriscerie I'm gonna try but this is my first contribution to a rust project and I'm not a expert in the language so I feeI might cause more noise than it's worth trying to apply the feedback.

If you want to co-author please go ahead.

selene/src/main.rs

Co-authored-by: boyned//Kampfkarren <boynedmaster@gmail.com>

…we searched both `.selene.toml` and `selene.toml` and none can be read.

…ctions to avoid code repetition.

This allow us to reuse 50% of the code.

Zeioth · 2024-03-18T16:01:09Z

Need changelog + docs.

Changelog: https://github.com/Zeioth/selene/blob/dot-selene-toml/CHANGELOG.md
Docs: Zeioth@e5fd4f0

More info

In the docs, I've just replaced every mention to selene.toml by .selene.toml since now .selene.toml has priority over selene.toml.

Only in src/usage/configuration we mention that we also accept selene.toml to avoid duplication.

Zeioth · 2024-03-18T16:03:10Z

The PR should be ready merge it. Please tell me if you would like some other changes.

Hopefully we can ship it relatively soon.

…ing.

chriscerie · 2024-03-23T16:48:11Z

Thanks for working on this! Unfortunately I've closed the parent issue for now: #469 (comment)

This pr will still be very helpful if the issue gets reopened in the future. If that happens, this pr will also need to update the vscode extension with the new config resolution.

chriscerie · 2024-03-26T16:34:39Z

docs/src/cli/usage.md

@@ -16,7 +16,7 @@ FLAGS:

 OPTIONS:
        --color <color>                     [default: auto]  [possible values: Always, Auto, Never]
-        --config <config>                  A toml file to configure the behavior of selene [default: selene.toml]
+        --config <config>                  A toml file to configure the behavior of selene [default: .selene.toml]


This isn't technically accurate because the default is both selene.toml and .selene.toml.

The default is .selene.toml because it has precedence over selene.toml,
The only place where selene.toml is mentioned as a possible alternative is docs/src/usagi/configuration to avoid duplicity that might make the docs harder to understand.

If you want to have a different string there, please tell me exactly, or edit the PR.

selene/src/main.rs

chriscerie

Also need relevant changes to vscode extension.

Co-authored-by: Chris Chang <51393127+chriscerie@users.noreply.github.com>

Zeioth · 2024-03-26T21:03:32Z

chriscerie since there are no bugs, the code has already being refactored as specified, and all the new reviews are stylistic preferences, or would affect parts outside the PR, I'm gonna leave the code as it is. Feel free to co-author if you see something you would prefer to do in a different way.

The code is safe to merge.

chriscerie · 2024-03-26T22:28:21Z

I appreciate your contribution, but this pr can't be merged until these bugs are fixed (see above comments for root causes). I'm happy to step in and co-author to get this merged but as there are other selene features with higher priority, I make no promises on the timeline.

Config with invalid utf-8 character

Currently

New

Missing config

Currently

New

…turned is not optional anymore, meaning it can't return None.

… the correct error message.

Zeioth · 2024-03-27T13:27:08Z

bad UTF-8 error is controlled now.

Zeioth · 2024-03-27T13:42:21Z

Missing file is also parsed correctly (this is my build, not the package manager one).

Zeioth · 2024-03-27T14:13:05Z

Relevant changes to vscode extension 47b0642

@chriscerie It should be ready but I have no clue how to test this on vscode. Could you test it or tell me how you test it so I can try to reproduce?

chriscerie

Missing config

Before

Now

Permission denied

Before

Now

selene/src/main.rs

… displaying a friendly message.

…rors -> report error and exit. * If path doesn't exist -> check next path. * None of the paths exist? → It uses the default config correctly.

…f-8. Plus, doing so would return a different error than the orignal.

…ng config file: No such file or directory (os error 2)`

…eading config file: No such file or directory (os error 2)`

✨feat: selene.toml and .selene.toml are not detected by order of …

6ad27f6

…priority.

Zeioth changed the title ~~✨feat: selene.toml and .selene.toml are not detected by order of priority~~ ✨feat: selene.toml and .selene.toml are now detected by order of priority Mar 10, 2024

chriscerie requested changes Mar 11, 2024

View reviewed changes

chriscerie linked an issue Mar 11, 2024 that may be closed by this pull request

Selene should search for configuration files that begin with a dot #469

Open

🐛fix(.selene): has now order of precedence over selene.toml.

02213f4

Zeioth changed the title ~~✨feat: selene.toml and .selene.toml are now detected by order of priority~~ ✨feat: .selene.toml and selene.toml are now detected by order of priority Mar 13, 2024

Zeioth added 2 commits March 13, 2024 02:48

🐛fix(error!): instead of eprintln.

b4a93e3

♻️refactor(read_config_file): Is now a function.

13e6286

Kampfkarren reviewed Mar 15, 2024

View reviewed changes

selene/src/main.rs Outdated Show resolved Hide resolved

Zeioth and others added 11 commits March 15, 2024 17:55

Update selene/src/main.rs

9e91ded

Co-authored-by: boyned//Kampfkarren <boynedmaster@gmail.com>

fix: Terminate the process when config file is invalid.

de9aa79

fix: We now check that the file exist. Also we only raise a error if …

8947dea

…we searched both `.selene.toml` and `selene.toml` and none can be read.

fix(regression): Ensure it fallback on the default config.

406558f

refactor of the former

f7785fb

♻️refactor: All should be fine now, but we can still refactor the fun…

5def22b

…ctions to avoid code repetition.

♻️refactor: All config file detection logic has been moved to functions.

69d6d98

This allow us to reuse 50% of the code.

comments: More consistent.

64bbb82

Update CHANGELOG.md

d0cb38e

docs: updated for .selene.toml.

e5fd4f0

Merge remote-tracking branch 'refs/remotes/origin/dot-selene-toml'

d7de32e

Zeioth requested review from Kampfkarren and chriscerie March 18, 2024 16:04

fix(comments): Our comments were preventing cargo rustdoc from runn…

1c425b8

…ing.

chriscerie closed this Mar 23, 2024

chriscerie reopened this Mar 26, 2024

chriscerie reviewed Mar 26, 2024

View reviewed changes

Zeioth and others added 3 commits March 26, 2024 18:16

Update selene/src/main.rs

0ef6e80

Co-authored-by: Chris Chang <51393127+chriscerie@users.noreply.github.com>

Update selene/src/main.rs

ef996c4

Co-authored-by: Chris Chang <51393127+chriscerie@users.noreply.github.com>

refactor: config_paths_to_check is now a constant.

822bc44

Zeioth added 2 commits March 27, 2024 14:00

refactor: read_config_file() -> (String, Option<PathBuf>) the type re…

ade463c

…turned is not optional anymore, meaning it can't return None.

refactor: Now that we handle the bad utf-8 error correctly, let's add…

3812d6b

… the correct error message.

vscode: relevant changes to vscode extension.

47b0642

chriscerie requested changes Apr 7, 2024

View reviewed changes

selene/src/main.rs Outdated Show resolved Hide resolved

selene/src/main.rs Outdated Show resolved Hide resolved

Zeioth and others added 10 commits May 7, 2024 18:18

♻️refactor: We now let rust manage the invalid UTF-8 error instead of…

4ca19d4

… displaying a friendly message.

🐛fix(config file detection): If path exists and fs::read_to_string er…

aff6f9a

…rors -> report error and exit. * If path doesn't exist -> check next path. * None of the paths exist? → It uses the default config correctly.

🐛refactor: There is no need for us to explicitely read the file as ut…

7c844a3

…f-8. Plus, doing so would return a different error than the orignal.

comments: Better info, in case we decide to enable this.

5c02fe9

fix: The error now works as originally intended → `ERROR: Error readi…

9829822

…ng config file: No such file or directory (os error 2)`

Update main.rs → comments

ed5e764

fix: Display the next error ONLY on validate-config → `ERROR: Error r…

7fa7825

…eading config file: No such file or directory (os error 2)`

Solve merge conflict

8817d13

Merge branch 'main' into dot-selene-toml

04ed488

Refactor

dcf3458

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨feat: `.selene.toml` and `selene.toml` are now detected by order of priority #591

✨feat: `.selene.toml` and `selene.toml` are now detected by order of priority #591

Zeioth commented Mar 10, 2024

Zeioth commented Mar 10, 2024

chriscerie left a comment

Zeioth commented Mar 11, 2024 •

edited

Loading

Zeioth commented Mar 18, 2024

Zeioth commented Mar 18, 2024

chriscerie commented Mar 23, 2024

chriscerie Mar 26, 2024

Zeioth Mar 26, 2024 •

edited

Loading

chriscerie left a comment

Zeioth commented Mar 26, 2024

chriscerie commented Mar 26, 2024

Zeioth commented Mar 27, 2024

Zeioth commented Mar 27, 2024

Zeioth commented Mar 27, 2024

chriscerie left a comment

✨feat: .selene.toml and selene.toml are now detected by order of priority #591

Are you sure you want to change the base?

✨feat: .selene.toml and selene.toml are now detected by order of priority #591

Conversation

Zeioth commented Mar 10, 2024

Zeioth commented Mar 10, 2024

chriscerie left a comment

Choose a reason for hiding this comment

Zeioth commented Mar 11, 2024 • edited Loading

Zeioth commented Mar 18, 2024

More info

Zeioth commented Mar 18, 2024

chriscerie commented Mar 23, 2024

chriscerie Mar 26, 2024

Choose a reason for hiding this comment

Zeioth Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

chriscerie left a comment

Choose a reason for hiding this comment

Zeioth commented Mar 26, 2024

chriscerie commented Mar 26, 2024

Config with invalid utf-8 character

Missing config

Zeioth commented Mar 27, 2024

Zeioth commented Mar 27, 2024

Zeioth commented Mar 27, 2024

chriscerie left a comment

Choose a reason for hiding this comment

✨feat: `.selene.toml` and `selene.toml` are now detected by order of priority #591

✨feat: `.selene.toml` and `selene.toml` are now detected by order of priority #591

Zeioth commented Mar 11, 2024 •

edited

Loading

Zeioth Mar 26, 2024 •

edited

Loading