zed icon indicating copy to clipboard operation
zed copied to clipboard

Add editorconfig support

Open BuonOmo opened this issue 1 year ago • 3 comments

Closes #8534

Release Notes:

  • Added EditorConfig support, overriding .zed/settings.json when a .editorconfig file is present

This is a first POC of editorconfig support. The main consideration to go further IMHO is: can we also store these settings as we do for settings per-language. This would likely be much more efficient. It might also require a more drastic change (maybe re-coding the ec4rs lib to use zed's filesystem), hence I wanted to already publish this for faster feedback by anyone interested in working along. As of now my goal was to make the minimal amount of changes possible

Tests could also be improved to cover every options.

BuonOmo avatar Aug 16 '24 11:08 BuonOmo

@SomeoneToIgnore I'm lacking some Cow knowledge. Could you help me with these failing tests ?

By the way, I'm off next week. I'll be back on that PR the 26 or 27.

BuonOmo avatar Aug 16 '24 14:08 BuonOmo

I am also quite occupied during this and next weeks, so great that we can postpone it — no rush, let's try again later.

SomeoneToIgnore avatar Aug 16 '24 14:08 SomeoneToIgnore

I think that besides perf, this is ready to ship. Should I focus on perf now (including file watching then) or shall we ship (once approved ofc!) and see this in another PR?

BuonOmo avatar Aug 17 '24 08:08 BuonOmo

I went on and implemented the main bits, but spoiled the tests. I see some odd access patterns of the whole thing when debugging, so will spend some time on fixing related things.

One confusing thing I've noticed is that [*.md]-like globs were ignored by the library(?): https://github.com/rust-lang/rust-analyzer/blob/095926ea6f008477a15a2ec6b0b8797e2e5be0e5/.editorconfig#L13 It would be interesting to know what's wrong with this.

SomeoneToIgnore avatar Aug 26 '24 16:08 SomeoneToIgnore

Ok, I've found a right place to put all new editorconfig data and fixed all issues.

The PR is getting ready, I have a concern left:

  • multiplayer support

Current code would not work with the remote workflow, as there's no real files and no FS scans on the client side, neither ec4rs::properties_of with its recursive FS ascend will work.

Seems that we have to actually copy the ConfigFiles::open code and resolve the configs manually. After everything's resolved, we'll push a similar to UpdateWorktreeSettings request from the host to clients, for them to populate their parsed editorconfig values.

  • ~WASM api seems to be using new extension data incorrectly: https://github.com/zed-industries/zed/pull/16349/files#diff-406f3ebd5f5aad8ecc4d8daf3e321b1d62b73848145a3f94f5b08a92f9c0dff5R402-R406~
  • ~overall, AllLanguageSettings::language is very hot, being called multiple times per second — the new code is adding some performance penalty on top, we need to think a bit more on ways to improve that and need to profile the code better~

SomeoneToIgnore avatar Aug 27 '24 18:08 SomeoneToIgnore

Back! Thank you for taking the time to implement, I feel a bit overwhelmed by your dev as I'm quite new to the Rust ecosystem. But if I can still help let me know!

Current code would not work with the remote workflow, as there's no real files and no FS scans on the client side, neither ec4rs::properties_of with its recursive FS ascend will work.

Then we could re-implement that method using zed's fs, couldn't we?

BuonOmo avatar Aug 29 '24 13:08 BuonOmo

It's harder than that, I'm afraid, as someone has to notify about things to the clients (FS has no way to know about real file changes and when to poll for them).

I can handle this PR from now on, if that's fine with you — the main, impact part is done thanks to you and now it's just some Rusty and Zed'y details I have to deal with. It might take some time though, as LanguageSettings is quite a foundational thing in our codebase and there's another task I have to tackle first.

SomeoneToIgnore avatar Aug 29 '24 14:08 SomeoneToIgnore

someone has to notify about things to the clients

How is that handled for .zed/settings.json files?

dbarnett avatar Sep 27 '24 15:09 dbarnett

This question would receive a different answer in August and today, as the part of code related to the whole settings sync is refactored actively due to ssh remoting.

So far, it landed into https://github.com/zed-industries/zed/blob/6d4ecac6100f7908278e78cb8c5102f7f91c54c5/crates/project/src/project_settings.rs#L202 and most probably stays this way, but I'd wait more before ssh remoting project finishes before ingraining anything .editorconfig-related as there could be more bugfixes and refactorings on top of the existing functionality.

So let me describe a general approach to the PR that would have been merged if submitted, on the example of settings.json processing and answer the question along the way.


The main idea is to watch files (in "offline" Zed locally on the host, in ssh and via-centralized-server (collab) Zed on the client) and gather updates into its memory. Directly on remote or through ssh client/collab server, eventually all that new/update data is supposed to land into https://github.com/zed-industries/zed/blob/6d4ecac6100f7908278e78cb8c5102f7f91c54c5/crates/settings/src/settings_store.rs#L161-L175

then user's Zed will call https://github.com/zed-industries/zed/blob/6d4ecac6100f7908278e78cb8c5102f7f91c54c5/crates/settings/src/settings_store.rs#L683

to parse and sync the resolved settings into its memory, and merge them in the end, in a generic fashion.

The language generic part is located in https://github.com/zed-industries/zed/blob/6d4ecac6100f7908278e78cb8c5102f7f91c54c5/crates/language/src/language_settings.rs#L879

We have to repeat a somewhat similar path for .editorconfig files:

  • on top, by reacting to file changes and storing the metadata bits similar to raw_local_settings from one the snippets above Per path, we'll have to honor .editorconfig data split by file, but otherwise seems to be the same "read the file into string" as with our json config.

This also means checking all remote workflows' sync events, to ensure we get that passed over to the client with the "real" editor.

  • then we'd need to merge those via recompute_values instead of recursive fs searches this PR does now, and keep in mind globs and root = true stop action, using the same stack-based approach recompute_values has for settings value recompute

  • last but not least, improve the bits I've added to get the language settings: https://github.com/zed-industries/zed/pull/16349/files#diff-9940b9338a85c5c6646ce6a8d2b3b7798845cbff70ec415d8df4c82d370efef2R800 and check that extensions pass the right path there, and Zed is able to

  • have I mentioned tests (there are 2 files named editor_tests.rs, one for "offline", another for "remote" tests, and we have to edit both. Ideally, we have to check that settings synchronized after external, possibly remote, editor changes, when one or multiple users edit random .editorconfig configs (in multiplayer tests) and language related (and not) files (verifying correctness in in singplayer tests)

  • more manual tests

SomeoneToIgnore avatar Sep 27 '24 16:09 SomeoneToIgnore

@SomeoneToIgnore this seems really fun to do. I'd like to be part of it if possible :)

BuonOmo avatar Sep 28 '24 10:09 BuonOmo

Why not, as this research PR is simpler to close than modify anyway at this state.

A change to track file changes and store raw file data for singleplayer & multiplayer modes could be made into a nice standalone PR.

SomeoneToIgnore avatar Sep 28 '24 14:09 SomeoneToIgnore

https://github.com/zed-industries/zed/pull/18616 altered a protocol and server DB level to support editorconfig sync messages. So now we can do another .ends_width similar to https://github.com/zed-industries/zed/blob/9cd42427d88afad3423fa2546ed656839295cf3f/crates/project/src/project_settings.rs#L401-L419 to update the raw settings storage.

SomeoneToIgnore avatar Oct 03 '24 06:10 SomeoneToIgnore

any news ?

ohroy avatar Oct 14 '24 02:10 ohroy

@BuonOmo can you check out https://github.com/zed-industries/zed/compare/kb/editorconfig?expand=1 branch? PR: https://github.com/zed-industries/zed/pull/19455 It lacks to be multiplayer test, but otherwise seems to be ready when I was testing it.

SomeoneToIgnore avatar Oct 18 '24 22:10 SomeoneToIgnore

@SomeoneToIgnore you were faster than me ! I think overall LGTM and you also already added some multiplayer test. I'd like to review a bit more the file crates/settings/src/settings_store.rs which is a bit hard to grasps to me. And I'll do some local test. All of this on Sunday afternoon and/or Tuesday :)

BuonOmo avatar Oct 19 '24 08:10 BuonOmo