Support constants #347
Support constants #347
Conversation
|
Seems to work(excluding the code style). Did I missed something @felixfbecker? |
Codecov Report
@@ Coverage Diff @@
## master #347 +/- ##
===========================================
- Coverage 84.2% 84.11% -0.1%
- Complexity 815 826 +11
===========================================
Files 59 59
Lines 1722 1712 -10
===========================================
- Hits 1450 1440 -10
Misses 272 272
|
|
any news on this? |
| { | ||
| // print TEST_PROPERTY ? 'true' : 'false'; | ||
| // Get hover for TEST_PROPERTY | ||
| $reference = $this->getReferenceLocations('TEST_PROPERTY')[0]; |
There was a problem hiding this comment.
Please name it TEST_DEFINE_CONSTANT or something like that, TEST_PROPERTY is confusing
| @@ -58,9 +58,10 @@ public function testEmptyQueryReturnsAllSymbols() | |||
| new SymbolInformation('TestInterface', SymbolKind::INTERFACE, $this->getDefinitionLocation('TestInterface'), ''), | |||
| new SymbolInformation('test_function', SymbolKind::FUNCTION, $this->getDefinitionLocation('test_function()'), ''), | |||
| new SymbolInformation('ChildClass', SymbolKind::CLASS_, $this->getDefinitionLocation('ChildClass'), ''), | |||
| new SymbolInformation('define', SymbolKind::CONSTANT, $this->getDefinitionLocation('TEST_PROPERTY'), ''), | |||
There was a problem hiding this comment.
If this is the defined constant, shouldn't the symbol name be TEST_PROPERTY (or TEST_DEFINE_CONSTANT), not define?
There was a problem hiding this comment.
No, because it's the name of it. The value is the TEST_PROPERTY. But I can change that if you want.
There was a problem hiding this comment.
What do you mean with value? A SymbolInformation has a name, and the name of the defined constant is TEST_PROPERTY. What value?
There was a problem hiding this comment.
ah, that's a bug. The name had been overwritten...
src/Protocol/SymbolInformation.php
Outdated
| && isset($node->args[0]) | ||
| && $node->args[0]->value instanceof Node\Scalar\String_ | ||
| ) { | ||
| $symbol->name = (string)$node->args[0]->value->value; |
There was a problem hiding this comment.
I'd added this because otherwise it would get renamed immediatly in line 98(105)
There was a problem hiding this comment.
l98 only sets containerName?
There was a problem hiding this comment.
In that line?
} else if (isset($node->name)) {
$symbol->name = (string)$node->name;
} I'd debugged a bit and figured out that without this method it will be set to define(which is the name of the node). But we want the value as a name, so that's why.
There was a problem hiding this comment.
You don't wanna check the exact same condition twice.
There was a problem hiding this comment.
I need to. Otherwise it would return null in this line: https://github.com/jens1o/php-language-server/blob/7380acc49fbacb4d5a6b3a7bf76d6dd55eed2043/src/Protocol/SymbolInformation.php#L88-L90
And otherwise it would get resetted when I don't check it twice.
There was a problem hiding this comment.
yeah, I see, but there are better options like setting the name early and checking later if the name is already set, or keeping a boolean flag.
|
Seems to work, did I missed something else? Would love to see this published asap, I need this in my personal project. 😄 |
|
Sorry for all these commits with the code style... I know my codestyle pretty well and forget to switch to other projects very often... |
src/Protocol/SymbolInformation.php
Outdated
| @@ -87,10 +99,13 @@ public static function fromNode(Node $node, string $fqn = null) | |||
| } else if ($node instanceof Node\Expr\ClosureUse) { | |||
| $symbol->name = $node->var; | |||
| } else if (isset($node->name)) { | |||
There was a problem hiding this comment.
this is equivalent to else if (isset($node->name) && !isset($symbol->name)
There was a problem hiding this comment.
Okay, I would do this like I did, for readability, but I changed it.
There was a problem hiding this comment.
Don't seems to be identical... This is what I meant. Should I revert it?
Yes, I used the master branch. So I have less problems 😉
resolves #84 and fixes #48