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.

[kirawi]

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. While this would make it so that Config is not EditorConfig-specific, I think it makes the code a lot cleaner.

What I would also do is make it so that, like is already done with language_loader, we could fill in Config from an EditorConfig if it exists (maybe from an Arc<EditorConfig>?). And then if using EditorConfig is undesirable, then it would just never fill in from an EditorConfig.

I'll think about this some more. I feel like there is a much better solution than what I proposed, using the Loader type to lazily load EditorConfigs.

[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: #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]

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. 😕

[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 #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

[fidelicura]

This PR is important if we want to improve edit experience for old-fashioned or legacy projects, especially C ones. Are there any progress?

[klardotsh]

On one hand I want to be sympathetic towards maintainers' ability and right to manage their spoons however they can and review things as they have time. It's unpaid (mostly?) volunteer effort, after all. On the other hand, this pull request for a feature that's fairly standard (either in core, or in plugins) in almost every non-boutique editor in the universe is over 2.5 years old and the OP is getting like zero timely (even on the scale of many months) engagement or feedback whatsoever. As an off-and-on user of Helix (a lot of the "off" time being due to missing last-mile problems like this one), it somewhat saddens me to see this. And I think it's a really rough public image for the project that may discourage future potential contributors from even bothering. I know I would certainly hesitate.

That would be fine if this were a "throw the code over the wall and don't really bring in outside code" repo - those repos are valid, too, not all FOSS has to be "a product". But Helix's development model and posture appears to be much more of "a product", and it purports to welcome outside contributions ("Contributors are very welcome! No contribution is too small and all contributions are valued."), so that discouragement seems.... bad?

[kirawi]

First of all, I apologize for not bringing up the status of this PR with the rest of the team beforehand. I've spoken with @pascalkuthe on Matrix and he expressed reservations about merging this PR because he feels that it is a very invasive change pulling in a significant dependency and not an essential enough feature to warrant being implemented in core. He would prefer to see this implemented as a plugin in the future.

To be clear, this only reflects his personal opinion and he has not discussed this with the other maintainers, so this is not set in stone. Nonetheless, I understand that this is going to be very frustrating to hear given the amount of effort the author put into this PR and how much time has passed since this was first opened. I also understand that many of you who were waiting to see this PR merged will feel justifiably frustrated as well.

I cannot speak on the behalf of the maintainers, but I acknowledge that we could have done better in communicating with the community. However, I think it is also important to understand, and I know many of you already do, that it is difficult to achieve that relationship with the community with consideration to how the maintainers are volunteering their time to the project. Of course, this doesn't invalidate any frustration, but I hope it can provide some insight into the response.

[TheDaemoness]

Very well. I appreciate the communication and hope that the overall maintainer opinion changes. I will leave this PR open but will not be updating it without an indication that it could plausibly be merged.

[blaggacao]

EditorConfig is an established standard.

Because it is impractical to get a .helix/languages.toml set up, leave alone upstreamed, for every (potentially random) repository helix users might want to contribute, lack of support for that standard is a practical hurdle to effective contribution, thus a marginal (!) hurdle to furthering the ideals of open source.

On average, it increases the time spent on preparing or reviewing a contribution for all users who find themselves in that situation.

[pickfire]

I don't see how this should be an associate trait, these data could be put within the struct itself.

[pickfire]

Can't both of this be in the same open function without generics?

[pickfire]

Sorry, I haven't been that active in helix as of lately (and have not discussed with the team about this PR). Personally, I do want to see this gets merged as I think it is a core feature like LSP. But I have some reservations the way the code and how ec4rs (it seemed slightly better since I last read it) is done.

My concerns is that the code being written is not that straightforward, slightly more complicated than necessary. To see this gets merged, most likely someone else have to come in and simplify stuff.

@TheDaemoness don't have to go update it first and probably get in touch with the other maintainers in the channel to know what is blocking the PR.

[dbarnett]

Fantastic to see an update here! 🎉

Are the code style concerns the same things commented inline, or could someone add comments if they're not? If @TheDaemoness isn't available for it I might be able to work on remaining touchups.

[kirawi]

As far as I'm aware, the other maintainers aside from Pascal have yet to comment on this PR so it definitely would be good to speak with them.

[the-mikedavis]

I agree with @pascalkuthe and I believe @archseer was thinking the same last time we discussed this

[TheDaemoness]

If I'm understanding the agreement correctly, that means the overall maintainer opinion is "it is a very invasive change pulling in a significant dependency and not an essential enough feature to warrant being implemented in core; would prefer to see this implemented as a plugin". Is there a rough estimate of when users can expect the plugin architecture to be in a state where it is capable of changing document options, including the encoding?

[kirawi]

I believe it is already able to, but cc @mattwparas

[dbarnett]

the overall maintainer opinion is "it is a very invasive change pulling in a significant dependency and not an essential enough feature to warrant being implemented in core ..."

FWIW it'd be very surprising to me to include language server support in core but not support for standard editor settings config. Or would language servers be moving into plugins as well...?

[kirawi]

From my understanding, Pascal preferred to add any options covered by editorconfig into Helix's config system if it didn't already exist.

[TheDaemoness]

From my understanding, Pascal preferred to add any options covered by editorconfig into Helix's config system if it didn't already exist.

This (emphasis mine) doesn't sound like expression of a preference to load .editorconfig files in Helix's config system to me.
I hope there's been a misunderstanding, because it sounds an awful lot like there is a preference to just have Helix's own way of configuring Helix, which defeats the entire point of EditorConfig (see blaggacao's comment).

It would be helpful if @pascalkuthe could clarify his preferences in this PR.

[the-mikedavis]

What Pascal is saying is that if there are missing config options that editorconfig exposes then we can add config options for those. For example insert_final_newline and #8157. Out of the box that would mean you could control those options with config.toml/languages.toml. Then an editorconfig plugin would be responsible for reading and parsing the editorconfig file and setting the equivalent options - the same way it works in Vim / Neovim for example.

[blaggacao]

Ok so, I understand a multiple PR approach:

1. Split out the part where new options are added and make them independently available to the helix config system. Rinse and repeat until all desired options are covered.
2. Prepare a version of this PR which can read the Editorconfig file and translate. Additional Bonus: make it a feature flag so that it can be used but doesn't commit the maintainer team for the long term in anticipation of a mandatory refactoring before graduating the feature once the config refactor landed.

There must be a way to not delay this feature any longer for affected users, while still preserving the legit long term interests of the maintainers. Or so I hope!

[the-mikedavis]

Point 1 sounds good regardless of editorconfig support. Though I think we have config options at least for what Vim / Neovim supports and what features we have. max_line_length is related to #2274 (not implemented), trim_trailing_whitespace is covered by #8366 and for spelling_language we don't have a spellchecker yet.

Point 2 is not what I had in mind though. We use feature flags for things like git that can be turned off to help compile to different targets like WASM: it's not meant to support features or dependencies we wouldn't otherwise support in core or to incubate / graduate features. If you want to use a feature that isn't supported in core (i.e. merged) you should run a branch from a PR or merge a PR's branch into a fork.

[TheDaemoness]

I assume the planned plugin system is https://github.com/mattwparas/helix/tree/steel-event-system (#8675) which appears to embed a novel Scheme interpreter to allow plugins to be written in Steel (a Scheme dialect). As far as I know there is no EditorConfig core in any dialect of Scheme, so it seems it would be necessary (or at least more practical) to make a Steel module out of an existing core.

I leave it to the maintainers to close this PR if it's decided this is the route they wish to go with. Best of luck to whoever opts to implement this plugin, and if I can be of assistance to that individual, I encourage them to reach out to me.