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

Unify and clarify config loading #5636

Closed
wants to merge 225 commits into from

Conversation

gibbz00
Copy link
Contributor

@gibbz00 gibbz00 commented Jan 22, 2023

The PR aims to reduce the omissions of necessary reliability checks when loading the configs (editor, theme, lang), whilst hopefully also improving code maintainability. All in all, xtask, helix_loader and helix_term::{main, appliction} have been cleaned up quite significantly. Steps are documented in the commit messages.

Relies on #5635

Identify and merge PRs that clash with this one.

My only gripe from #5411 was that it now places hx --grammar (fetch|build) files into XDG_CONFIG_HOME (.config/), under the condition that it exists. Would much rather prefer it to look in XDG_DATA_HOME (.local/share) first. Which is something I've added in this PR. This might be a solution to #584, and an alternative to #5603. In fact, #3202 is the improvement suggested by #584's author:

Even better solution would include Helix merging config and grammars from system dirs and user dirs and having fallbacks configurable at buildtime but meh. This should be enough for most usecases.

Does also not change log file location to an OS state directory, as it may not exist on all platforms.

Suggestions:

  • Remove doc comments in editor.rs, they're out of sync with configuration.md. Alternatively generate configugration.md from them.

Conflicting PRs (28)

#5411 #5435 #5436 #5491 #5523 #5577 #5608 #5621 #5645 #5660 #5748 #5803 #5839 #5860 #5893 #5931 #6056 #6059 #6061 #6065 #6067 #4307 #4443 #4493 #5199 #2869 #3328 #3393
* That are mergeable with master and pass all CI checks as of 2023-02-22

AlexanderBrevig and others added 30 commits August 30, 2022 08:42
Co-authored-by: Michael Davis <mcarsondavis@gmail.com>
This is an implementation for helix-editor#3346.

Previously, one of the following runtime directories were used:

1. `$HELIX_RUNTIME`
2. sibling directory to `$CARGO_MANIFEST_DIR`
3. subdirectory of user config directory
4. subdirectory of path to helix executable

The first directory provided / found to exist in this order was used as a
root for all runtime file searches (grammars, themes, queries).

This change lowers the priority of `$HELIX_RUNTIME` so that the user
config runtime has higher priority. More significantly, all of these
directories are now searched for runtime files, enabling a user to override
default or system-level runtime files. If the same file name appears
in multiple runtime directories, the following priority is now used:

1. sibling directory to `$CARGO_MANIFEST_DIR`
2. subdirectory of user config directory
3. `$HELIX_RUNTIME`
4. subdirectory of path to helix executable

One exception to this rule is that a user can have a `themes`
directory directly in the user config directory that has higher piority
to `themes` directories in runtime directories. That behaviour has been
preserved.

As part of implementing this feature `theme::Loader` was simplified
and the cycle detection logic of the theme inheritance was improved to
cover more cases and to be more explicit.
@archseer
Copy link
Member

I'd prefer PRs didn't bundle other PRs. While #5411 will get merged I'd rather review it as a standalone unit since it's much more manageable to land one by one.

HELIX_LOG_LEVEL env variable can now also be set in a non integration
test environment.
Stupidly hard to differentiate it with config::Config.
A step in making it more in line to how the other configs are loaded.
paves the way for a Default implementation for Theme
@gibbz00 gibbz00 force-pushed the unify-config branch 2 times, most recently from 5e9d0a1 to cf98312 Compare February 16, 2023 20:10
* Allow override of predifinde infobox descriptions.
* Make sure that custom description overrider on infoboxes can be done
without additional remappings of the same keytrie.
@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 17, 2023

I'd prefer PRs didn't bundle other PRs. While #5411 will get merged I'd rather review it as a standalone unit since it's much more manageable to land one by one.

Fair. Til next time ✌️ Will the "bundling" disappear from this PR on Github once #5411 gets merged?

@gibbz00 gibbz00 marked this pull request as ready for review February 17, 2023 00:44
@gibbz00
Copy link
Contributor Author

gibbz00 commented Feb 17, 2023

How should I treat PRs such as: #5839?

It heavily conflicts with this one. Merge it now or only when/if the other is merged into master?

@gibbz00 gibbz00 mentioned this pull request Feb 22, 2023
@flokli
Copy link

flokli commented Mar 29, 2023

@gibbz00 with #5411 merged, can you rebase, and possibly clean up the history a bit? This should make it easier to review.

@gibbz00
Copy link
Contributor Author

gibbz00 commented Mar 29, 2023

@gibbz00 with #5411 merged, can you rebase, and possibly clean up the history a bit? This should make it easier to review.

I have no idea what the status is on #5635, which this PR builds upon. And given how my other PRs / discussions have. been. received. , I've moved my focus to other places.

@pascalkuthe
Copy link
Member

Closing as stale

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-helix-term Area: Helix term improvements S-waiting-on-pr Status: This is waiting on another PR to be merged first
Projects
None yet
Development

Successfully merging this pull request may close these issues.