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

Improve handling of qualified names when renaming #531

Merged
merged 65 commits into from
Jan 8, 2025

Conversation

toinehartman
Copy link
Contributor

@toinehartman toinehartman commented Nov 26, 2024

  • Support renaming fully qualified modules names (instead of last name segment only)
  • Reject renaming of modules from qualified declaration names.
  • Explicitly do not support external imports
  • Support module renames from explorer view.
  • Escape name segments (\syntax) and v.v.
  • Resolve module names via dependencies
  • Use 'did rename' instead of 'will rename' callback, to prevent problems in case the evaluator is locked
  • Add UI test for module move
  • Change qualified names on folder rename

@toinehartman toinehartman self-assigned this Nov 26, 2024
@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch from 2e00c0b to 046df83 Compare November 26, 2024 16:25
@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch 2 times, most recently from 63a2866 to 9f8d64b Compare December 2, 2024 11:50
@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch 2 times, most recently from 83be677 to 9dc8e7d Compare December 5, 2024 10:50
Copy link
Member

@DavyLandman DavyLandman 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, with some exceptions that I marked down.

@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch from 05284c1 to dd91729 Compare December 9, 2024 13:08
@toinehartman toinehartman marked this pull request as ready for review December 10, 2024 14:00
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

Some small changes, looks mostly fine 👍🏼

Copy link
Member

@DavyLandman DavyLandman 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, 2 points that we should still pay attention to:

  1. the work inside the eval closure should really only call the evaluator, nothing more (just to avoid deadlocks), I've left a comment where it's happening.
  2. the test currently fails.

@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch 2 times, most recently from 308fcd9 to 5491327 Compare December 16, 2024 15:42
Copy link
Member

@DavyLandman DavyLandman left a comment

Choose a reason for hiding this comment

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

Some minor comments, but looks good. Also thinks @sungshik for the help with the UI tests.

@toinehartman toinehartman force-pushed the fix/rename-refactoring/module-names branch 2 times, most recently from a3e18f6 to a262066 Compare December 23, 2024 14:15
@DavyLandman
Copy link
Member

@toinehartman looks like it's ready to merge, want to do the honors?

@toinehartman
Copy link
Contributor Author

Just before the holidays, I found there is still an issue remaining with this. A module rename from an editor triggers a move of a module, among other changes. That move than triggers the rename 'from the explorer' (that this PR implements). The latter then produces some edits which are applied after the edits from the from-editor rename, resulting in faulty qualified names.

I switched around the order of the edits that a rename from editor produces in an attempt to reduce this - apply the edits, first, and moves later. For some simple examples, that seems to work, but it's not a robust solution. It seems that versioned edits would get rid of this issue somewhat, although the race for which is applied first, might still be a problem there.

I'll investigate a bit more.

@toinehartman
Copy link
Contributor Author

After off-line discussion, we reasoned that the issue is mostly solved by reordering the edits that renamings produce. Some async behaviour will remaing due to the nature of the LSP, and is acceptable. Merging.

Copy link

sonarqubecloud bot commented Jan 8, 2025

@toinehartman toinehartman merged commit 257f075 into main Jan 8, 2025
14 checks passed
@toinehartman toinehartman deleted the fix/rename-refactoring/module-names branch January 8, 2025 08:57
toinehartman added a commit that referenced this pull request Jan 8, 2025
…le-names

Improve handling of qualified names when renaming

(cherry picked from commit 257f075)
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.

3 participants