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

Add editorconfig support #16349

Closed
wants to merge 12 commits into from
Closed

Conversation

BuonOmo
Copy link
Contributor

@BuonOmo BuonOmo commented Aug 16, 2024

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.

@cla-bot cla-bot bot added the cla-signed The user has signed the Contributor License Agreement label Aug 16, 2024
@BuonOmo BuonOmo force-pushed the editorconfig branch 3 times, most recently from f759684 to dbfc560 Compare August 16, 2024 12:11
@maxdeviant maxdeviant changed the title feat: Add editorconfig support Add editorconfig support Aug 16, 2024
Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks nice, I think that will cover most of the .editorconfig demand and is quite small.

Besides the comments left, there are two important things I'd love to see in this PR:

  • instead of changing existing tests extensively, a new test for all properties overridden in .editorconfig, to ensure we are able to apply "all" of them, and, maybe in the same test, an external fs edit scenario, to ensure Zed is able to update its state accordingly

  • a file watching, as you've noted in your comment

crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Aug 16, 2024

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

@SomeoneToIgnore
Copy link
Contributor

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

Comment on lines +131 to +138
let fs = FakeFs::new(cx.executor());
fs.insert_tree_from_real_fs(path, path).await;
let project = Project::test(fs, [path], cx).await;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wanted to use:

Suggested change
let fs = FakeFs::new(cx.executor());
fs.insert_tree_from_real_fs(path, path).await;
let project = Project::test(fs, [path], cx).await;
let project = Project::test(Arc::new(RealFs::default()), [path], cx).await;

But somehow the .zed/settings file is not detected that way... Anyone knows what I am missing ?

@BuonOmo BuonOmo force-pushed the editorconfig branch 2 times, most recently from bd55820 to 1587660 Compare August 17, 2024 08:38
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Aug 17, 2024

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?

Copy link
Contributor

@SomeoneToIgnore SomeoneToIgnore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have left more comments and generally disagree that we have to ship soon: for now, it's not working correctly yet and the perf issue seems relatively straightforward to fix.

I have already mentioned that we should add the file watching and I do not see why not — left a more elaborate comment in the related thread.

crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
crates/language/src/language_settings.rs Outdated Show resolved Hide resolved
@SomeoneToIgnore
Copy link
Contributor

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
Copy link
Contributor

SomeoneToIgnore commented Aug 27, 2024

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.

@BuonOmo
Copy link
Contributor Author

BuonOmo commented Aug 29, 2024

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?

@SomeoneToIgnore
Copy link
Contributor

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 SomeoneToIgnore force-pushed the editorconfig branch 2 times, most recently from a0dfae7 to c57f03d Compare September 7, 2024 14:38
@SomeoneToIgnore SomeoneToIgnore force-pushed the editorconfig branch 2 times, most recently from eaae206 to ff97c84 Compare September 17, 2024 04:24
Kiwow added a commit to Kiwow/work that referenced this pull request Sep 24, 2024
This is mainly to fix Zed indent guides using 2 spaces when the project
uses 4.

It can be converted to an .editorconfig when
zed-industries/zed#16349 is merged.
@dbarnett
Copy link

someone has to notify about things to the clients

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

@SomeoneToIgnore
Copy link
Contributor

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

pub struct SettingsObserver {
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

pub struct SettingsStore {
setting_values: HashMap<TypeId, Box<dyn AnySettingValue>>,
raw_default_settings: serde_json::Value,
raw_user_settings: serde_json::Value,
raw_extension_settings: serde_json::Value,
raw_local_settings: BTreeMap<(WorktreeId, Arc<Path>), serde_json::Value>,
tab_size_callback: Option<(
TypeId,
Box<dyn Fn(&dyn Any) -> Option<usize> + Send + Sync + 'static>,
)>,
_setting_file_updates: Task<()>,
setting_file_updates_tx: mpsc::UnboundedSender<
Box<dyn FnOnce(AsyncAppContext) -> LocalBoxFuture<'static, Result<()>>>,
>,
}

then user's Zed will call

fn recompute_values(

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

impl settings::Settings for AllLanguageSettings {

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 SomeoneToIgnore mentioned this pull request Sep 27, 2024
@BuonOmo
Copy link
Contributor Author

BuonOmo commented Sep 28, 2024

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

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Sep 28, 2024

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
Copy link
Contributor

#18616 altered a protocol and server DB level to support editorconfig sync messages.
So now we can do another .ends_width similar to

if path.ends_with(local_settings_file_relative_path()) {
let settings_dir = Arc::from(
path.ancestors()
.nth(local_settings_file_relative_path().components().count())
.unwrap(),
);
let fs = fs.clone();
settings_contents.push(async move {
(
settings_dir,
LocalSettingsKind::Settings,
if removed {
None
} else {
Some(async move { fs.load(&abs_path).await }.await)
},
)
});
}
to update the raw settings storage.

@ohroy

This comment has been minimized.

@SomeoneToIgnore
Copy link
Contributor

SomeoneToIgnore commented Oct 18, 2024

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

@BuonOmo
Copy link
Contributor Author

BuonOmo commented Oct 19, 2024

@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 BuonOmo deleted the editorconfig branch October 20, 2024 16:04
SomeoneToIgnore added a commit that referenced this pull request Oct 21, 2024
Closes #8534
Supersedes #16349

Potential concerns:
* we do not follow up to the `/` when looking for `.editorconfig`, only
up to the worktree root.
Seems fine for most of the cases, and the rest should be solved
generically later, as the same issue exists for settings.json
* `fn language` in `AllLanguageSettings` is very hot, called very
frequently during rendering. We accumulate and parse all `.editorconfig`
file contents beforehand, but have to go over globs and match these
against the path given + merge the properties still.
This does not seem to be very bad, but needs more testing and
potentially some extra caching.


Release Notes:

- Added .editorconfig support

---------

Co-authored-by: Ulysse Buonomo <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla-signed The user has signed the Contributor License Agreement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

EditorConfig
5 participants