Implement EditorConfig support

[TheDaemoness]

Closes #279.

This changeset currently implements partial support for EditorConfig. Here's what it includes:

• Full support for EditorConfig's globbing style.
• The indent_size, indent_style, tab_width, charset, and end_of_line properties.

Here's what it doesn't include:

• Support for non-standard charset values.
• insert_final_newline or trim_whitespace. Both of these should be possible to do with some changes to Document::save_impl, but a good implementation would allow these to be controlled from outside EditorConfig as well, and I think it would be better to do this in a separate PR.
• Some editors support max_line_length for doing hard wrapping. Helix does not.

[kirawi]

I'm not familiar with EditorConfig, but I think that it makes more sense to deserialize into a Rust struct (maybe one of the existing Config structs?) and then operate on that, to generalize to other forms of configuration.

[TheDaemoness]

I'm similarly unfamiliar with Helix's internals, so please correct me if any of my assumptions about them are wrong.

helix_term::Config appears to control editor-wide settings, such as the theme and what language servers should be used for what language.

That sort of configuration is out-of-scope for EditorConfig, which is more about declaring properties of individual files in a project, such as their encoding, line endings, indent style, maximum line length, formatting, and so on. The idea is that a project can put an .editorconfig file in its root, and contributors to that project can have their editors auto-configure themselves to comply with that project's style guidelines.

From what I could find, most of the variables that an EditorConfig file would call for changes to, in Helix, are in helix_view::Document. I wanted to do the parsing in Document::open and not Editor::open because the presence of an EditorConfig file potentially defeats the need for certain auto-detection that Helix does, and because parsing can be skipped if the file exists but cannot be opened.

Of course, if generalizing to other ways of setting these variables per-file is still important, that could still be done. I could define an object-safe trait and a couple of implementors for different forms of per-file editor configuration. It would probably keep the code of Document::open_with_config (or w/e) cleaner as well.

[TheDaemoness]

I think that since we're already creating a Config type for Document, it might make more sense to encapsulate the already existing configuration fields for Document in Config.

That sounds good to me. One small quirk: Currently the fields of Config are Options, which they shouldn't be if a Config is going to be stored in a Document. However, if there's going to be some lazily-loaded EditorConfig type (or ConfigLoader or w/e), this isn't an issue at all. There might need to be some refactoring of the detect_*_impl methods to accommodate this.

I'll think about this some more.

Alright, I'll hold off on implementing any of these ideas for now.

[kirawi]

I think for now just lazily loading the configs based on the Document's language should be fine. If that's too complicated, then I'm fine without it too. My other suggestion to merge the config of the Document into one type might make things more complicated than they need to be.

Unless there's a reason it's not possible, I think it would be better to choose the config based on the language the Document is set to, rather than basing it on the path. This could be done by using the file-types field in languages.toml, with a fallback to using the path if no language has such a file-type.

[TheDaemoness]

Alright! I can finally prioritize this again! Apologies, I know it's been a while.

I think for now just lazily loading the configs based on the Document's language should be fine.

Unless there's a reason it's not possible, I think it would be better to choose the config based on the language the Document is set to, rather than basing it on the path

I think something got lost in communication. EditorConfig's purpose is to automate reconfiguring one's editor to match the style guidelines of a project, at least for certain common settings. It has no concept of language, and any concept of language it gains in the future would likely (speculation on my part) be in the form of a new key-value pair to specify that files matching a certain pattern are in a certain language. After all, language detection (and naming!) varies significantly from editor to editor. In short, what you're proposing is no longer EditorConfig. Sorry!

My other suggestion to merge the config of the Document into one type might make things more complicated than they need to be.

Alright, noted. I'll take a look at what's changed and try see how much of what I wrote can be adapted. I still need to finish ec4rs 1.0.0 before moving this out of draft status. I'll try and have something ready for review this weekend. If not this weekend, then the next.

[kirawi]

EditorConfig has globs for file extensions, and the Language config contains the extension as well. In retrospect, it would only make sense to look at the config if the buffer wasn't saved to a file yet. But I suppose it doesn't matter and goes against the config's model as it is only working on file extensions.

[TheDaemoness]

From the latest run:

error: package ec4rs v1.0.0 cannot be built because it requires rustc 1.59 or newer, while the currently active rustc version is 1.57.0

Is Rust 1.57 a hard requirement? ec4rs uses 1.59 because of destructuring assignments, but I should be able to put out a patch release that's compatible with an earlier version.

EDIT: It's been two weeks. I'm going to assume the answer is "yes". I'll try and push changes this weekend and mark this as ready for review so that this feature can get merged in some form.

[the-mikedavis]

Yep sry, we use 1.57 as an MSRV because it makes builds easier for distros like Void that might be a bit behind on their rust version: https://github.com/helix-editor/helix/pull/1881

[TheDaemoness]

EditorConfig specifies properties for inserting a newline at the end of the file if there isn't one already, as well as a trimming trailing whitespace after newlines. As far as I can tell, Helix doesn't allow one to control either of these things normally, meaning there's room to add these as a feature independent of EditorConfig. Should support for them be in a different PR?

If not, let me know and I can just implement them. Otherwise, marking this for review.

[the-mikedavis]

Yeah, implementing those can be separate 👍

We probably want to add commands for both of those things - they're definitely useful even if they aren't automatic/on-save

[TheDaemoness]

What is the status of review of this PR? I really would rather not have to keep fixing merge conflicts so that it can be merged as-reviewed.

[the-mikedavis]

You don't need to fix the merge conflicts - we can resolve those before merging

[dbarnett]

Cool, looks like this is still up-to-date, just waiting on another round of review before helix can support editorconfig files?

[dhollinger]

Any updates on this? Our team relies on editorconfig to dynamically configure our editors used by the team, so this is something I'm waiting for.

[TheDaemoness]

I'm still around and watching this PR if changes need to be made.

[TheDaemoness]

Alright. Pushed what's (hopefully) a much cleaner implementation.

A few other things that have changed as a result of this:

• The logic for opening Documents for files that don't exist is slightly different. Instead of calling Path::exists, the new logic attempts to open the file and handles NotFound as a special case.
• The document preview in the terminal picker now respects EditorConfig settings, including encoding and tab width.
• For files that do not exist, the rope is initialized with a newline matching the one specified by EditorConfig.
• Returning from the previous implementation is a tab_width field in Document, which might be useful for PRs like #561.

[nathan-at-least]

I'm new to EditorConfig and have some usability suggestions/questions (that may be general to EC, but hx needs to have an answer). These may be addressable through multiple PRs (I don't think it makes sense to bog down the "mvp" PRs with these).

UI Feedback

It would be nice if there is UI feedback when EditorConfig settings apply to a given buffer, so that users may hopefully be less confused why something deviates from their explicit hx config settings.

An extra bonus if there's some way to examine config settings which apply to a buffer and the source of the config (ie: config.toml, EditorConfig, language settings, etc…). (Does hx already have a settings browser? I'm aware of :get-option.)

Merge rules

How are "native" hx configs and EditorConfig settings merged?

In some cases where there's obvious contention, EC should likely be preferred, for example the indent level or whether to use tabs or spaces for indent.

In other cases, a heuristic of "use the 'stricter' of the two config sources for every setting" might make sense. For example, if hx had an option to strip trailing whitespace automatically, then if an EC does not specify stripping trailing whitespace on *.rs files, hx may be designed to strip trailing whitespace on an EC file.

OTOH, maybe it's simpler, easier to understand, and safer to have a simpler rule that's along the lines of "for every hx native setting, if there is an analogous EC setting, then if EC is active, it always overrides the hx native setting".

Thoughts?

Overriding EditorConfig

Can a user choose to override configuration set by EditorConfig?

Does there need to be new commands or settings to facilitate this?

Simple approach: introduce a global config for EC support. If a user wants to opt-out (and thus potentially violate a project's standards) they can disable this option.

I would advocate for making this setting default to true, even though it may surprise some users, because I'm a big fan of automating team coordination by default.

[TheDaemoness]

Two of these questions have answers as of this PR!

How are "native" hx configs and EditorConfig settings merged?

Effectively, EditorConfig settings override "native" Helix settings wherever there is a conflict. For each setting, Helix checks the set of EditorConfig settings that apply to that file first (unless EditorConfig support is disabled), then falls back to "native" Helix settings, and then back to a sensible default if those aren't set either (e.g. for indentation).

So, in the example of Helix gaining a setting to strip trailing whitespace automatically and EditorConfig not specifying it for Rust files, Helix would use its "native" setting for that.

Can a user choose to override configuration set by EditorConfig?

This PR adds an editorconfig boolean option to helix_view::editor::Config which defaults to true. It could, in a future PR, be changed to some type that is a list/two lists of path prefixes for which EditorConfig is enabled/disabled, but can also be deserialized from boolean values to maintain compatibility.

[zqianem]

With #8157 recently merged, would it be a good idea to now support EditorConfig's insert_final_newline in this PR?

[TheDaemoness]

With #8157 recently merged, would it be a good idea to now support EditorConfig's insert_final_newline in this PR?

I would be up for this, but I would also appreciate a sign of interest (or concern!) from the maintainers before committing to this branch further, given that it has been just over 11 months since the last message from a collaborator/member in this PR. :confused:

[tennox]

I would also appreciate a sign of interest (or concern!) from the maintainers before committing to this branch further

Could you give some guidance/resonance? @the-mikedavis @archseer

[daedroza]

I would also like to see this feature, unsure why the silence let alone inactivity from mergers?

[kirawi]

This PR will likely be impacted by https://github.com/helix-editor/helix/issues/8853

[TheDaemoness]

This PR will likely be impacted by #8853

Should I take this to mean that the changes proposed in #8853 will likely be merged before this PR?

[kirawi]

That's up to the maintainers, but either way the implementation here would be affected (here or in a subsequent PR) if that RFC goes forward

[sehqlr]

I hope this gets supported, but if it doesn't, there should be a tool for converting an editorconfig file into a helix language.toml file