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

Support for EditorConfig #279

Open
cessen opened this issue Jun 16, 2021 · 21 comments · May be fixed by #1777
Open

Support for EditorConfig #279

cessen opened this issue Jun 16, 2021 · 21 comments · May be fixed by #1777
Labels
A-helix-term Area: Helix term improvements C-enhancement Category: Improvements

Comments

@cessen
Copy link
Contributor

cessen commented Jun 16, 2021

In #239 it was mentioned that support for EditorConfig would be nice, and it seemed like there was general agreement that we want to do that at some point.

I'm opening this issue just so it stays on our radar.

@pickfire
Copy link
Contributor

pickfire commented Jun 16, 2021

While we do that, I think we want to have a logo first to be able to put our editor in editorconfig.org

Should we use https://github.com/mrandri19/rust-editorconfig?

@pickfire pickfire added the C-enhancement Category: Improvements label Jun 16, 2021
@sudormrfbin
Copy link
Member

It doesn't seem to be maintained so we might have to pull it in and make some changes.

@pickfire
Copy link
Contributor

Maybe we can take over the maintainership of the crate?

@sudormrfbin
Copy link
Member

There's an open issue for that too (offering maintainership): mrandri19/rust-editorconfig#13

@archseer
Copy link
Member

I wonder if we could just attempt to parse it via TOML, it shares some similarities with INI.

@cessen
Copy link
Contributor Author

cessen commented Jun 17, 2021

There's also serde_ini, which seems like a reasonable way to go since we already have the serde dependency anyway.

@kirawi
Copy link
Member

kirawi commented Sep 9, 2021

It might just make sense to adapt https://github.com/zonyitoo/rust-ini. It seems like the only difference between .ini and .editorconfig are wildcard patterns in the section names.

Actually, I wonder if we need to do that at all and can just use it directly.

Edit:
I checked it out and it seems like we can just use that crate directly without modifying anything. We just need to eliminate any duplicates (and only retain the first parsed value). This should be easy since the values are in the order they were parsed.

@sudormrfbin
Copy link
Member

Dropping this here for future reference if we go for the manual implementation using an ini crate route, there is an editorconfig-plugin-tests repository containing instructions and sample files for testing.

@kirawi kirawi added the E-medium Call for participation: Experience needed to fix: Medium / intermediate label Sep 11, 2021
@kirawi
Copy link
Member

kirawi commented Sep 11, 2021

Worst case scenario, we can just bind to https://github.com/editorconfig/editorconfig-core-c. But I don't think we'll need to.

@kirawi kirawi self-assigned this Dec 9, 2021
@TheDaemoness
Copy link

What is the status of this feature's implementation? If there is a lack of manpower for it, then I'd be happy to contribute some.

@kirawi
Copy link
Member

kirawi commented Feb 12, 2022

Feel free :) I had some IRL things happen that prevented me from tackling this.

@kirawi kirawi removed their assignment Feb 12, 2022
@TheDaemoness
Copy link

Is there a preference for wrapping editorconfig-core-c or writing something new in pure Rust? The spec seems simple enough for now.

@kirawi
Copy link
Member

kirawi commented Feb 18, 2022

https://github.com/zonyitoo/rust-ini seems to be able to parse it just fine.

@TheDaemoness
Copy link

I'll take that as a preference for writing a pure Rust implementation. That was my preference as well. 👍

rust-ini appears unfit for this purpose, specifically because its section parser ends parsing at the first ] matched. That's fine for most INI, but Editorconfig allows regex-style character ranges in sections.

Either way, I think the largest challenge in writing a new Editorconfig implementation will be the globing behavior.

From there, integrating it doesn't seem particularly involved:

  • Changing the indent style, character encoding, and line ending seem like they can be done with some calls in Document::open.
  • Changing the tab width could be done by adding an Option<usize> field to Document, and making Document::tab_width check that first before falling back to its current behavior. Let me know if there's a better approach.
  • Trimming trailing whitespace and inserting a final newline if one doesn't exist is harder. It seems like the place to do this is in Document::save_impl after the LSP formatting changes are applied.
  • Helix doesn't currently support hard-wrapping (see Command to hard-wrap selected text. #625), so the non-standard max_line_length property will be ignored for now.

@TheDaemoness
Copy link

TheDaemoness commented Mar 4, 2022

It's been just over two weeks, so here's a status update.

I've been writing a new EditorConfig core. At the time of writing, it passes 161 of the 196 core tests, and some of the glob-related code quality is not what I'd like it to be (can't use the glob crate, unfortunately).

Once it passes all of the tests, I'd like to publish a 1.0.0 release to crates.io and add it as a dependency of Helix. Its only dependency is on std, but in the future it might also depend on unicode-segmentation. Alternatively, since I'm not expecting its public API to change as I bring the library into full test compliance, I could make a 0.3.0 release after fixing its main issues, and use that in Helix. Then, when 1.0.0 is done, all that should be necessary is a version bump. I expect it should work pretty well in the real world even without full compliance. Let me know!

@yoav-lavi
Copy link
Contributor

I'll add that .editorconfig files also aren't highlighted at the moment, not sure if that's planned as a part of this issue

@danillos
Copy link
Contributor

danillos commented Aug 28, 2023

I'll add that .editorconfig files also aren't highlighted at the moment, not sure if that's planned as a part of this issue

It already has a PR and mention to this PR. #8076

@yoav-lavi
Copy link
Contributor

Highlighting temporarily added as INI in #8308

@kirawi kirawi removed the E-medium Call for participation: Experience needed to fix: Medium / intermediate label Feb 26, 2024
@koalalorenzo
Copy link

Damn, I hope this will do the cut for the next release!

@christ-offer
Copy link

Fingers crossed, this would be a very useful feature!

@noor-tg
Copy link

noor-tg commented Aug 23, 2024

so no .editorconfig support until now for helix ?

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 C-enhancement Category: Improvements
Projects
None yet
Development

Successfully merging a pull request may close this issue.