jmacdonald / amp

A complete text editor for your terminal.
https://amp.rs
Other
3.73k stars 105 forks source link

Add .editorconfig support #116

Closed christoph-heiss closed 3 years ago

christoph-heiss commented 5 years ago

First off I just want to say that I really like amp! It already has replaced vim for me!

Anyway, this addresses #18.

Currently, only indent_style, indent_size, tab_width, trim_trailing_whitespace and insert_final_newline are supported. I also added an .editorconfig file for this repo.

(tab_width needs still some work, this setting is not yet respected in the renderer.)

I'm sending this now to get some comments on the current implementation (if it is ok), and further to discuss how to handle the end_of_line and charset properties. From what I saw, there is currently no real handling for different file-encodings (everyone should be using UTF-8 anyway but it is part of .editorconfig spec). Same goes for line-endings handling.

Also some tests still need to be written to test the .editorconfig functionality.

jmacdonald commented 5 years ago

@christoph-heiss thanks, glad to hear that! :smile:

I haven't had time to do a review yet, but I'll set aside some time this weekend; will reach out soon. :sunglasses:

christoph-heiss commented 4 years ago

After about 1.5 years, I picked this patchset up again. I now wrote our completely own parser for .editorconfig files without relying on any additional external crates. This gives us more flexibility in the end too, which might be a good thing for amp.

Now, as discussed previously, it now will only load the .editorconfig file in the workspace directory and ignore nested configurations completely (they are probably so rare anyway that this will hardly be ever an issue).

There is still one big TODO (handling nested numeric ranges), and more test cases should also be added. But again, weird nesting of ranges is probably a rare case and not used often.

Respecting tab_width in the renderer is also still not implemented, as noted in the original PR. Edit: It is respected when rendering. I just didn't test this properly.

So IMHO this probably could be merged as-is if the current implementation is otherwise fine, but I'm open about how to move forward with this.

jmacdonald commented 4 years ago

@christoph-heiss so glad you've revisited this! I'm going to set aside some time over the next week to properly review; will comment shortly. :slightly_smiling_face:

christoph-heiss commented 3 years ago

Hi! I took another shot at this - and revamped this whole thing a bit. So now, instead of using the globset crate, the .editorconfig file gets directly parsed into Regex's for matching.

Since the globset crate was using regex under the hood anyway, this saves us the wonky process of correctly converting the .editorconfig glob for globset, giving us more control over everything. Now we go glob -> regex directly instead of glob -> globset -> regex.

The regex patterns for the individual glob-parts were directly taken from the editorconfig/editorconfig-core-c. I also wrote a pretty exhaustive test suite for everything on the way, which helps a lot with validation.

jmacdonald commented 3 years ago

Thanks for this @christoph-heiss! I know I'd promised to review this earlier in the year, but things went sideways and my spare time mostly vanished. I'm getting back into the swing of things now and will revisit this next; thanks for being patient. :smile:

christoph-heiss commented 3 years ago

No worry, review it as your time allows; this year has been hard on everyone.

soloturn commented 3 years ago

@jmacdonald this is cool, can you merge please?

@christoph-heiss line endings are practical in rare cases, e.g. makefiles need to have linux line endings, and windows command files need to have windows line endings.

christoph-heiss commented 3 years ago

I've (finally) implemented all your review comments; definitely all very senseful suggestions. 😅

Moving the parser into a separate crate isn't a bad idea (especially since there is basically none maintained as you say), but I think it would make sense to at least first flesh out all the details until it is "shippable". I can still extract it later on anyway, but for now it would probably be more overhead than necessary.