Feat: Rename imports #119

predragnikolic · 2022-01-05T10:11:20Z

When we rename or move a file,
the user is prompted to rename all imports.

This PR also adds a new setting updateImportsOnFileMove with the following options "prompt", "always", "never".

predragnikolic · 2022-01-05T10:15:05Z

Notes:

I haven't tested this on Mac/Windows.
This feature will work only when we rename one file. There are some File Managers that allows renaming multiple files at once,
but this feature wont work in cases like that.

rename_imports.py

rchl

Also a bunch of pyright and mypy issues in those changes.

rename_imports.py

rchl · 2022-01-05T10:48:35Z

rename_imports.py

+    if len(queued_events) == 2:
+        first_event, new_name = queued_events[0]
+        second_event, old_name = queued_events[1]
+        if (first_event == 'create' and second_event == 'delete'):


I'm afraid that there is not really a reliable way to detect renames. It could be just two unrelated files being added and removed (for example when changing branches).

I thought that chokidar has a way to detect a rename, but that is not the case.
there is a different lib, that supports rename detection => paulmillr/chokidar#303 (comment)

For the time being, I would rely on this logic:

if len(self.events) == 2: create_event = next((event for event in self.events if event[0] == 'create'), None) delete_event = next((event for event in self.events if event[0] == 'delete'), None) if (create_event and delete_event): # Rename detected new_name = create_event[1] old_name = delete_event[1] self.prompt_rename(old_name, new_name)

If we have 2 events,
and when we have a create and delete event,
only then prompt the rename....

It could be just two unrelated files being added and removed (for example when changing branches).

While than can happen,
I do not know how to better handle that at the moment.

I thought that chokidar has a way to detect a rename, but that is not the case.
there is a different lib, that supports rename detection => paulmillr/chokidar#303 (comment)

Interesting watcher. At least if we are to trust the author's claims.

I wouldn't want to switch to using it as that's too much work and the claims would have to be tested in practice also but we could take inspiration from it and implement support for rename events in our watcher.

I do not know how to better to handle that than at the moment.

Me neither that's why I'm suggesting to remove the potentially dangerous option to rename without confirmation.

there is a different lib, that supports rename detection => paulmillr/chokidar#303 (comment)

BTW. The code style of that project is... interesting.

Blank lines between (almost) every line of code

excessive use of spaces (leading/trailing spaces within parenthesis for example)

https://github.com/fabiospampinato/watcher/blob/master/src/watcher.ts

It would not be fun to contribute to such project :)

rename_imports.py

rchl · 2022-01-05T10:53:28Z

plugin.py

+        if workspace_folders:
+            register_rename_import_file_watcher(workspace_folders[0].path)


Should be done for every folder?

I plan to add support form multiple folders,
I just need more time to think about how to handle the cleanup logic (when one folder closes, and so on)

I was thinking of having a Map, where the key is the root_path and the value is a RenameImportHandler instance.

How would you know if/when the folders have changed?

Multiple folders are semi supported.

The current code already supports sublime project.
When having a ST project with multiple folders:

// example.sublime-project { "folders": [ { "path": "/home/predragnikolic/Documents/sandbox/my-react-ts-app" }, { "path": "/home/predragnikolic/Documents/sandbox/coc-tsserver" }, { "path": "/home/predragnikolic/Documents/sandbox/volar" }, { "path": "/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript" } ] }

It works until the moment you try to add/remove folders to a project while ST is open. :)

The current code only takes the path of the first folder.

plugin.py

rename_imports.py

rchl · 2022-01-05T19:34:47Z

rename_imports.py

+        if len(self.events) == 2:
+            create_event = next((event for event in self.events if event[0] == 'create'), None)
+            delete_event = next((event for event in self.events if event[0] == 'delete'), None)


This seems very fragile and implementation dependent. The logic should just be: if there are two sibling events where one is 'create' and one is 'delete' then trigger the rename.

Same for the timeout that calls this method. There is no guarantee that both events will come within 10ms and if they don't, we'll clear the list and miss the whole rename.

And to clarify, I meant without checking if there are exactly 2 events as that is not guaranteed.

The logic should just be: if there are two sibling events where one is 'create' and one is 'delete' then trigger the rename.

We cannot rely on sibling events. :)

Here are the logs when renaming multiple files at once on Linux, .
the folder structure is the following:

├── test │ ├── file1.ts │ ├── file2.ts │ └── file3.ts

if we rename multiple files at once,
we will have the following events logged:

events [('delete', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file1.ts')] events [('delete', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file2.ts')] events [('delete', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file3.ts')] events [('create', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file11.ts')] events [('create', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file21.ts')] events [('create', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file31.ts')]

see the gif

So to jump to my point is,
we cannot rely on two sibilling events,
as that would result in incorrect behavior.

In the above example, it would mean to rename file3.ts to file11.ts, which is not correct.

events [('delete', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file3.ts')] events [('create', '/home/predragnikolic/.config/sublime-text/Packages/LSP-typescript/test/file11.ts')]

In that case it seems like we need more sophisticated solution. Something similar to what is mentioned in the link you've posted before (paulmillr/chokidar#303 (comment))

I also can imagine that 3 renames in a row would not be handled well by the quick panel that we are triggering.

I intentionally didn't want to support multiple file renames at once. only because I do not want to spam the user will prompts.

I only wanted to cover the case when a user renames a single file.

In that case it seems like we need more sophisticated solution. Something similar to what is mentioned in the link you've posted before ...

Will improve the logic by checkin the iNode number of a file. :)

well,
I tried to use os.stat(file_name).st_ino to get the ino number for a file.

But I quickly run into problems.

Here was my soloution:

events_with_ino = [(event[0], event[1], os.stat(event[1]).st_ino) for event in events] # ^^^^^^^^ event_type # ^^^^^^^^ file_name # ^^^^^^^^^^^^^^^^^^^^^^^^ ino number for that file_name

I tried to attach an ino number for a file_name,
but this works only for files that exist.

That said os.stat(event[1]).st_ino will work only for create or change events,
but not for when the event type is delete.
Because by the time we receive the delete event in python,
the file no longer exists,
so calling os.stat(file_name_that_no_longer_exist).st_ino will throw.

Ideally I would shift this logic to be the responsibility of the file_watcher,
so we have an interface like this:
https://github.com/neoclide/coc-tsserver/blob/master/src/server/features/updatePathOnRename.ts#L33

Supporting this in the watcher is the way to go but not sure if we would be able to do it in LSP-file-watcher-chokidar. Might need to be implemented upstream in chokidar instead.

rename_imports.py

plugin.py

instead register it on_ready, that way I avoid the confusion I had with self/cls

…enameOfImportPath

rchl · 2022-01-11T08:18:04Z

plugin.py

        super().__init__(*args, **kwargs)

+    def register_rename_import_file_watcher(self, session: Session) -> None:
+        allow_rename_of_import_path = session.config.init_options.get('preferences.allowRenameOfImportPath')
+        workspace_folders = session.get_workspace_folders()


This variable is only needed with the if file_watcher condition.

rchl · 2023-02-21T17:37:09Z

Quite a lot of work was put into it but ultimately I don't think that this is the way to go. It would not be fine to trigger disrupting rename prompts whenever something would be renamed. Renames should only be handled by the server when user explicitly asks to rename some file so we need LSP-specific rename actions instead.

Maybe if there would be a way to offer the user suggestion to update imports in a non-distracting way then it would be more acceptable but it might be better to just only do that on explicit request to rename.

predragnikolic · 2023-02-21T21:40:00Z

Agreed, I will close this PR.

I saw the willRename PR on the ts server repo.
I believe that is the way to go.

rename imports

dd9b068

predragnikolic commented Jan 5, 2022

View reviewed changes

rename_imports.py Outdated Show resolved Hide resolved

rchl requested changes Jan 5, 2022

View reviewed changes

predragnikolic added 2 commits January 5, 2022 12:40

fix styles and return watcher

c30ec4d

rethought feature based on feedback

ae157d9

predragnikolic commented Jan 5, 2022

View reviewed changes

plugin.py Show resolved Hide resolved

rchl requested changes Jan 5, 2022

View reviewed changes

predragnikolic added 4 commits January 6, 2022 14:28

do not try to register the file watcher in is_allowed_to_start

0c6a61a

instead register it on_ready, that way I avoid the confusion I had with self/cls

remove parenthesis

c77bae4

rename HandleRenameImport -> RenameImportHandler

df4f2be

remove updateImportsOnFileMove setting in favor of preferences.allowR…

03a33e9

…enameOfImportPath

rchl reviewed Jan 11, 2022

View reviewed changes

predragnikolic closed this Feb 21, 2023

rchl deleted the feat/rename-imports branch February 21, 2023 21:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat: Rename imports #119

Feat: Rename imports #119

predragnikolic commented Jan 5, 2022

predragnikolic commented Jan 5, 2022

rchl left a comment

rchl Jan 5, 2022

predragnikolic Jan 5, 2022 •

edited

Loading

rchl Jan 5, 2022

rchl Jan 5, 2022 •

edited

Loading

rchl Jan 5, 2022

predragnikolic Jan 6, 2022

rchl Jan 6, 2022 •

edited

Loading

predragnikolic Jan 10, 2022

rchl Jan 11, 2022

rchl Jan 5, 2022

rchl Jan 5, 2022

predragnikolic Jan 6, 2022

rchl Jan 6, 2022

rchl Jan 6, 2022

predragnikolic Jan 10, 2022

predragnikolic Jan 10, 2022

predragnikolic Jan 10, 2022

rchl Jan 11, 2022

rchl Jan 11, 2022

rchl commented Feb 21, 2023 •

edited

Loading

predragnikolic commented Feb 21, 2023

		if workspace_folders:
		register_rename_import_file_watcher(workspace_folders[0].path)

Feat: Rename imports #119

Feat: Rename imports #119

Conversation

predragnikolic commented Jan 5, 2022

predragnikolic commented Jan 5, 2022

rchl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

predragnikolic Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Jan 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl Jan 6, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rchl commented Feb 21, 2023 • edited Loading

predragnikolic commented Feb 21, 2023

predragnikolic Jan 5, 2022 •

edited

Loading

rchl Jan 5, 2022 •

edited

Loading

rchl Jan 6, 2022 •

edited

Loading

rchl commented Feb 21, 2023 •

edited

Loading