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

[GH-8286] Fix Go to Declaration for an enum method using an FQ name #8287

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

NReib
Copy link

@NReib NReib commented Mar 2, 2025

#8286
This is my first push using git.

Sadly I was not able to create unit tests for this.

@troizet troizet added the PHP [ci] enable extra PHP tests (php/php.editor) label Mar 2, 2025
@troizet troizet requested review from tmysik and junichi11 March 2, 2025 12:07
@junichi11
Copy link
Member

junichi11 commented Mar 4, 2025

use fully qualified name when extracting variable type

It's not easy to understand what is fixed. So, I would write the following to the commit message title instead. (Also an example)

"[GH-8286] Fix Go to Declaration for an enum method using an FQ name"

Example:

namespace test1;

enum TestA {

    case X;

    public function get(string $param): string {
        return '';
    }
}

namespace test2;

\test1\TestA::X->get('xxx'); // don't work go to declaration & mark occurrences for the get method

Sadly I was not able to create unit tests for this.

It's possible. Please add unit tests for GotoDeclaration and MarkOccurrences. (GotoDeclarationPHP81Test or GotoDeclarationGH8286Test)

Please add the issue number(e.g. [GH-8286]) or URL to the commit message. Thanks!

@junichi11 junichi11 added this to the NB26 milestone Mar 4, 2025
@junichi11 junichi11 linked an issue Mar 4, 2025 that may be closed by this pull request
@NReib
Copy link
Author

NReib commented Mar 4, 2025

@junichi11 Thank you for your feedback. It seems I misunderstood something about the unit tests. I added 2 test cases and changed the implementation due to failing tests. I hope it is more clear now what is happening.
It appears I have reformatted the code of VariousUtils, is there any Formatting Guideline active for NB?

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Please don't use TABs.
Missing ***.occurrences file.

@junichi11
Copy link
Member

It appears I have reformatted the code of VariousUtils, is there any Formatting Guideline active for NB?

Basically just follow the existing source code. Please reformat only your changes.

@junichi11 junichi11 added do not merge Don't merge this PR, it is not ready or just demonstration purposes. Need Squashing labels Mar 4, 2025
@junichi11
Copy link
Member

You have to squash as one commit.
As I wrote above, we should add an example to the commit message.

[GH-8286] Fix Go to Declaration for an enum method using an FQ name

Example:
```php
namespace test1;

enum TestA {

    case X;

    public function get(string $param): string {
        return '';
    }
}

namespace test2;

\test1\TestA::X->get('xxx'); // don't work go to declaration & mark occurrences for the get method
``` 

@NReib
Copy link
Author

NReib commented Mar 5, 2025

@junichi11 squashed, changed formatting, added result files

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Please use 4 spaces.

@junichi11 junichi11 removed do not merge Don't merge this PR, it is not ready or just demonstration purposes. Need Squashing labels Mar 6, 2025
@NReib NReib force-pushed the B8286 branch 2 times, most recently from 39903ac to 88a3a98 Compare March 6, 2025 08:20
…name

-Accessing a Constant of an Enum/Class now uses FQ name if possible

Example:
```php
namespace test;

enum TestA{

        case X;

        public function get(string $param): string{
                return '';
        }
}

namespace test2;

enum TestB{

        case X;

        public function get(): string{
                return '';
        }
}

\test\TestA::X->get('xxx'); // go to declaration and mark occurences do not work
```
@NReib
Copy link
Author

NReib commented Mar 6, 2025

@junichi11 changed to 4 spaces. Is there any correct way to push these kind of changes while keeping it in the same commit? Now I have been using --force, although I have heard it is not good to do that

@junichi11
Copy link
Member

junichi11 commented Mar 7, 2025

This should be no problem because it is a topic branch and we can see the diff(https://github.com/apache/netbeans/compare/88a3a985ba388732459deb98aa83c406755ead1b..65cb2cf61af76acf8b1e587186e1562a75d8d3f6).

  • fix something
  • git commit --amend
  • force push

or

  • fix something
  • git commit
  • fix something
  • git commit
  • ...
  • review is done
  • squash them as one commit
  • force push

Copy link
Member

@junichi11 junichi11 left a comment

Choose a reason for hiding this comment

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

Thank you for your work!

@junichi11
Copy link
Member

Please also change the title of this PR. Thanks!

@NReib NReib changed the title use fully qualified name when extracting variable type [GH-8286] Fix Go to Declaration for an enum method using an FQ name Mar 7, 2025
@NReib NReib changed the title [GH-8286] Fix Go to Declaration for an enum method using an FQ name Fix Go to Declaration for an enum method using an FQ name Mar 7, 2025
@tmysik
Copy link
Member

tmysik commented Mar 8, 2025

@NReib, thanks for this PR! 🎉

Regarding Git force-pushing - typically, it is totally OK, if you are the only person working on the branch. It should/cannot be used when more people work on it, since it basically rewrites the Git history (so, if user A force-pushes something, it could overwrite - and therefore loose - changes done by someone else).

@tmysik
Copy link
Member

tmysik commented Mar 8, 2025

@NReib, as @junichi11 wrote, please always try to unify the issue and the PR so it is clear that they belong to each other. For the issue, I can see this:

image

Also, providing a code example in the PR description helps a lot.

Thank you.

@tmysik tmysik changed the title Fix Go to Declaration for an enum method using an FQ name [GH-8286] Fix Go to Declaration for an enum method using an FQ name Mar 8, 2025
@tmysik
Copy link
Member

tmysik commented Mar 8, 2025

(I changed the PR title now.)

Copy link
Member

@tmysik tmysik left a comment

Choose a reason for hiding this comment

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

Thank you for this PR. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PHP [ci] enable extra PHP tests (php/php.editor)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[PHP] Enum method not found
4 participants