-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Add editorconfig support #16349
Conversation
f759684
to
dbfc560
Compare
There was a problem hiding this 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
@SomeoneToIgnore I'm lacking some By the way, I'm off next week. I'll be back on that PR the 26 or 27. |
I am also quite occupied during this and next weeks, so great that we can postpone it — no rush, let's try again later. |
let fs = FakeFs::new(cx.executor()); | ||
fs.insert_tree_from_real_fs(path, path).await; | ||
let project = Project::test(fs, [path], cx).await; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wanted to use:
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 ?
bd55820
to
1587660
Compare
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? |
There was a problem hiding this 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.
I went on and implemented the main bits, but spoiled the tests. One confusing thing I've noticed is that |
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:
Current code would not work with the remote workflow, as there's no real files and no FS scans on the client side, neither Seems that we have to actually copy the
|
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!
Then we could re-implement that method using zed's fs, couldn't we? |
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. |
a0dfae7
to
c57f03d
Compare
eaae206
to
ff97c84
Compare
ff97c84
to
938f1ac
Compare
938f1ac
to
c048993
Compare
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.
How is that handled for |
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 zed/crates/project/src/project_settings.rs Line 202 in 6d4ecac
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. zed/crates/settings/src/settings_store.rs Lines 161 to 175 in 6d4ecac
then user's Zed will call zed/crates/settings/src/settings_store.rs Line 683 in 6d4ecac
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 zed/crates/language/src/language_settings.rs Line 879 in 6d4ecac
We have to repeat a somewhat similar path for .editorconfig files:
This also means checking all remote workflows' sync events, to ensure we get that passed over to the client with the "real" editor.
|
@SomeoneToIgnore this seems really fun to do. I'd like to be part of it if possible :) |
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. |
#18616 altered a protocol and server DB level to support editorconfig sync messages. zed/crates/project/src/project_settings.rs Lines 401 to 419 in 9cd4242
|
This comment has been minimized.
This comment has been minimized.
@BuonOmo |
@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 |
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]>
Closes #8534
Release Notes:
.zed/settings.json
when a.editorconfig
file is presentThis 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.