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

Enable rename plugin #2598

Closed
wants to merge 2 commits into from
Closed

Enable rename plugin #2598

wants to merge 2 commits into from

Conversation

jneira
Copy link
Member

@jneira jneira commented Jan 17, 2022

@jneira jneira requested a review from michaelpj January 17, 2022 07:18
@@ -18,6 +18,8 @@ You can watch demos for some of these features [below](#demos).
- [Call hierarchy support](#call-hierarchy)
- [Qualify names from an import declaration](#qualify-imported-names) in your code
- [Suggest alternate numeric formats](#alternate-number-formatting)
- [Renaming](#renaming) definitions automatically across modules of *the same component* (library, executable, test suite, benchmark).
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is not correct. Renaming works across all modules that have been indexed (in this or previous sessions).

Copy link
Member Author

Choose a reason for hiding this comment

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

Aha, thanks, please, could you note in which cases the lack of multicomponent support could affect the plugin behaviour? does it work for the components currently loaded? so if you open files from components forcing their loading, the renaming would work across those loaded components (and no others)?

Copy link
Collaborator

Choose a reason for hiding this comment

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

could you note in which cases the lack of multicomponent support could affect the plugin behaviour?

I already said: the plugin will rename all occurrences that have been indexed in this or previous IDE sessions.

One example where the plugin would fail to find some occurrences is the first time a project has been loaded in the IDE ever, and only a subset of the components are loaded.

does it work for the components currently loaded?

It works for all the components indexed, whether they are loaded or not.

so if you open files from components forcing their loading, the renaming would work across those loaded components (and no others)?

If you open a file from a component, that will cause the component to be fully indexed. So assuming that you wait until indexing is done, the plugin will work for that component.

Copy link
Member Author

@jneira jneira Jan 17, 2022

Choose a reason for hiding this comment

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

Ok so indexed as indexed in hiedb, i guess, thanks for the clarification
Afaiu, as we have the option haskell.checkProject, hls by default would index the entire the project opening any file from it, right?
If it is set to false the best way to ensure the last version of files are indexes would open all files where the symbol to berenamed i present.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Afaiu, as we have the option haskell.checkProject, hls by default would index the entire the project opening any file from it, right?

If by project you mean component, then yes.

If it is set to false the best way to ensure the last version of files are indexes would open all files where the symbol to berenamed i present.

If it is set to false, components won't be indexed in full on open. So you could just open all the files in the workspace.

Copy link
Collaborator

@michaelpj michaelpj left a comment

Choose a reason for hiding this comment

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

approve modulo Pepe being happy about us getting the caveats right.

@OliverMadine
Copy link
Collaborator

OliverMadine commented Jan 17, 2022

I think that the caveat should be that it only works for single-component projects. It can have issues with single-components within multi-component projects.

Indexing all components of files where the symbol is referenced before renaming should work (I did this manually when recording the example gif).
We could grep the entire project for the symbol to do this (since reference finding also won't work yet), however, we would need to be careful not to index files that don't have a proper cradle.

@jneira
Copy link
Member Author

jneira commented Jan 17, 2022

Hmm, i would prefer to have a partial but consistent behaviour. If it depends on what files has been opened by the user, and the renaming is partial and potentially different between sessions the ux could be very bad.

@OliverMadine so the fact hls checks the entire project would not make renaming work without opening all the files with symbol occurences?

Afaiu, as we have the option haskell.checkProject, hls by default would index the entire the project opening any file from it, right?

@OliverMadine
Copy link
Collaborator

@OliverMadine so the fact hls checks thenetire projectwould not make renaming work without opening all the files with symbol occurences?

I've just tested by deleting my cache and trying to rename. It does not seem to index the entire project, but only the components for open files.

@jneira
Copy link
Member Author

jneira commented Jan 17, 2022

Thanks, so i am not sure anymore if we can enable it at compile time without disabling by default at runtime. It would be acceptable ask for open all files with the symbol to make it work? i am sure there will be uses surprised to see the renaming works some times and other only partially.

@michaelpj thoughts?

@michaelpj
Copy link
Collaborator

Okay, I give in, that does seem bad enough that we shouldn't do it. Do we have any path forwards for fixing the underlying problem?

@michaelpj
Copy link
Collaborator

Can we write a rule that indexes everything in the project and then have rename depend on that? It'll be expensive but at least it will work.

@jneira
Copy link
Member Author

jneira commented Jan 18, 2022

It seems hls does not know about all project components beforehand. The components are loaded via cradles on demand, while files are being opened.
I guess @OliverMadine plan is do just that but hls needs #2009 to load all components in the startup, as noted in the issue.

@OliverMadine
Copy link
Collaborator

OliverMadine commented Jan 18, 2022

It seems hls does not know about all project components beforehand. The components are loaded via cradles on demand, while files are being opened.
I guess @OliverMadine plan is do just that but hls needs #2009 to load all components in the startup, as noted in the issue.

Yes.

I think that if we assume that the hie.yaml lists all the components, then we can extract them from there. We need to be able to load multiple components (which I believe was the purpose of #304).

So as you said, we just need #2009.

@jneira jneira marked this pull request as draft January 30, 2022 15:42
@jneira
Copy link
Member Author

jneira commented Feb 28, 2022

dont have time to continue working on this, feel free to reuse it when enabling definitle the plugin

@jneira jneira closed this Feb 28, 2022
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.

4 participants