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

Rename support #768

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Rename support #768

wants to merge 8 commits into from

Conversation

rummatee
Copy link

This PR attempts to implement the rename function. Therefore it implements #21 .
There are however still some limitations:
If there isn't a reference in the same file, as the definition, the definition doesn't get changed.
Also at least in my editor (nvim) it is only possible to do one rename and it's necessary to restart before doing the next rename.
I would appreciate any help with these issues or any further comments.

@codecov
Copy link

codecov bot commented Nov 15, 2019

Codecov Report

Merging #768 into master will decrease coverage by 1.05%.
The diff coverage is 52.7%.

@@             Coverage Diff              @@
##             master     #768      +/-   ##
============================================
- Coverage     82.12%   81.07%   -1.06%     
- Complexity      935      955      +20     
============================================
  Files            44       44              
  Lines          2154     2219      +65     
============================================
+ Hits           1769     1799      +30     
- Misses          385      420      +35
Impacted Files Coverage Δ Complexity Δ
src/Definition.php 100% <ø> (ø) 6 <0> (ø) ⬇️
src/Index/Index.php 75% <0%> (-2.42%) 56 <0> (+2)
src/Factory/LocationFactory.php 100% <100%> (ø) 2 <1> (+1) ⬆️
src/DefinitionResolver.php 87.58% <100%> (+0.12%) 335 <0> (+3) ⬆️
src/Server/TextDocument.php 64.83% <44.64%> (-10.73%) 71 <36> (+14)

Copy link
Owner

@felixfbecker felixfbecker left a comment

Choose a reason for hiding this comment

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

Thanks for taking a stab at this! However, the issues in the description seem like blockers before merging this.

@felixfbecker felixfbecker changed the title Rename Rename support Nov 16, 2019
@rummatee
Copy link
Author

I rewrote the definition finding and added code to update the index after the rename. The issues I described earlier should be solved with this.

@@ -423,6 +426,9 @@ private function indexDefinition(int $level, array $parts, array &$storage, Defi
*/
private function removeIndexedDefinition(int $level, array $parts, array &$storage, array &$rootStorage)
{
if (empty($parts)) {
Copy link
Author

Choose a reason for hiding this comment

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

I don't really understand this function well enough to be confident about this code, but it fixes an exception that I got before and it seems intuitively right to have a base case for the recursion.

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

Successfully merging this pull request may close these issues.

2 participants