Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Appearance settings

Linked edit fixes #1947

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

Merged
merged 8 commits into from
Jan 15, 2025
Merged

Linked edit fixes #1947

merged 8 commits into from
Jan 15, 2025

Conversation

paulbutcher
Copy link
Contributor

@paulbutcher paulbutcher commented Jan 14, 2025

  • I created an issue to discuss the problem I am trying to solve or an open issue already exists.
  • I added a new entry to CHANGELOG.md
  • I updated documentation if applicable (docs folder)

Fixes #1943

This is a work in progress: it makes things better than they currently are, but definitely still isn't a complete solution.

This PR:

However it still doesn't work entirely reliably (the timing-related issue described here is still evident). I'm also not really happy with the fact that I've had to cobble together alias-range instead of using ->range (as far as I can see there's no other way to dig the alias portion out of a symbol?).

I'll see if I can work out what's going on with the timing issue, but I'm still very unsure whether the relatively little value added by linked edits for Clojure is worth the trouble? 🤷

@ericdallo
Copy link
Member

(as far as I can see there's no other way to dig the alias portion out of a symbol?).

Unfortunately, yes

I'll see if I can work out what's going on with the timing issue, but I'm still very unsure whether the relatively little value added by linked edits for Clojure is worth the trouble? 🤷

I think the problem is the analysis we lost when we change the buffer, when we edit anything, clojure-lsp sends to clj-kondo the whole text to get new analysis, changing its internal db (atom), I believe there is some issue while the analysis was not saved to the db, but that usually means the request are in some weird order

@paulbutcher
Copy link
Contributor Author

I think the problem is the analysis we lost when we change the buffer, when we edit anything, clojure-lsp sends to clj-kondo the whole text to get new analysis, changing its internal db (atom), I believe there is some issue while the analysis was not saved to the db, but that usually means the request are in some weird order

That makes sense: I can see that find-element-under-cursor is sometimes returning nil if it's called "soon" after an edit. What can I do about this?

@paulbutcher
Copy link
Contributor Author

@ericdallo I've pushed changes which should fix the integration tests.

In the process, I discovered that assert-submap is sensitive to the order of the maps, which surprised me (and feels a bit fragile?). But anyway, I think this should now be good to go.

@ericdallo
Copy link
Member

That makes sense: I can see that find-element-under-cursor is sometimes returning nil if it's called "soon" after an edit. What can I do about this?

@paulbutcher I think I have the fix for this, we need to wrap in handlers.clj the linkedEdit call with process-after-all-changes, this may fix the concurrence issue, could you try?

Copy link
Member

@ericdallo ericdallo left a comment

Choose a reason for hiding this comment

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

Looks good, thank you!

@ericdallo ericdallo merged commit 23e51d9 into clojure-lsp:master Jan 15, 2025
11 of 14 checks passed
acamargo pushed a commit that referenced this pull request Mar 18, 2025
* Restrict linked edits to namespace aliases only

* Use alias-range instead of ->range

* Remove debugging erroneously left in

* Fix integration tests

* Fix lint

* Update changelog
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Restrict linked edits to namespace aliases only
2 participants
Morty Proxy This is a proxified and sanitized view of the page, visit original site.