[FEATURE] Output changelog url in OutputFormatterInterface #6073
Conversation
|
I am cheating here. I don´t know why or how i should use the phpstan ClassReflection here. Any ideas? |
| { | ||
| $rectorClass = get_class($this->rector); | ||
|
|
||
| /* @phpstan-ignore-next-line */ |
There was a problem hiding this comment.
I am cheating here. I don´t know why or how i should use the phpstan ClassReflection here. Any ideas?
That is false positive, because it's Rector internal class the reflection is used on. But using the ClassReflection class on some projects class. e.g. Typo3\...\Form would disable static reflection. I'll try to update it, so it won't bother contributors in the future :) thanks for feedback.
We put ignores to phsptan.neon, to keep track on them and avoid code polution with tool ignores. Could you move it there?
|
Thanks for the PR! I have troubles understanding, because "changelog" reminds me Could you provide some real life example how you use it and what happens? Gif is worth thousand words :) vendor/bin/rector ...
↓
output (screenshot) |
|
|
|
Changelog has nothing to do in our case with the Changelog.md file. We are creating for every change in TYPO3 a really nice changelog file in the rst format transformed to html. Example: https://docs.typo3.org/c/typo3/cms-core/master/en-us/Changelog/9.4/Deprecation-85793-SeveralConstantsFromSystemEnvironmentBuilder.html. In TYPO3 Rector i add for every rector a nice @link block to reference to this url. This is really nice for the consumer to see why a certain rule is applied. |
|
That´s the json output |
sabbelasichon
force-pushed
the
issue-6072
branch
from
fafab22
to
b4b9ed4
Compare
sabbelasichon
force-pushed
the
issue-6072
branch
from
b4b9ed4
to
daffe55
Compare
|
I see. So this way we can learn more detailed desription of the change in particular framework? We actually do something like this in implicit way: We could enable this by default, as there is no reason to hide this information. No configuration option is needed. One more thing. Currently the JSON output is working with classes only in "applied_rectors". That way it can be used in Rector demo page to link exact rule:
To keep the original value and add a new one, there should be a new value: {
"applied_rules": ["SomeRule"],
"applied_rules_with_changelog": {
"SomeRule": "https://project/change/link.md"
}
} |
|
@TomasVotruba I am not 100% sure about the tests. I have introduced some tests to verify the behaviour. Am i right, that no tests exists before to ensure the desired behaviour? Do i miss something? |
| $rectorClass = get_class($this->rector); | ||
|
|
||
| $rectorReflection = new ReflectionClass($rectorClass); | ||
|
|
||
| $docComment = $rectorReflection->getDocComment(); | ||
|
|
||
| if (! is_string($docComment)) { | ||
| return null; | ||
| } | ||
|
|
||
| $pattern = "#@link\s*(?<url>[a-zA-Z0-9, ()_].*)#"; | ||
| preg_match($pattern, $docComment, $matches); | ||
|
|
||
| if (! array_key_exists('url', $matches)) { | ||
| return null; | ||
| } | ||
|
|
||
| if (! filter_var($matches['url'], FILTER_VALIDATE_URL)) { | ||
| return null; | ||
| } | ||
|
|
||
| return trim((string) $matches['url']); |
There was a problem hiding this comment.
This seems quite a complext logic for simple value object.
Could you extract this to own service and cover it with simple test?
$extractedValue = $this->extractAnnotationFromClass('SomeRector::class', '@link');
$this->assertSame('...', $extractedValue);There was a problem hiding this comment.
Where should this service be located? Is there not already something similar we could use?
|
These are the first test to covert this part, so I think it's ok 👍 It's getting into good shape 👍 I wonder about last think - the annotation name, @see, @link and @source seems a bit unclear. Now that you explained the purpose, maybe the name This feature would be a big addition to understand what rules do and why 👍 |
|
Is @changelog a de-facto official docblock? But, maybe we should not care about it and invent our own. |
👍 I think it's good name for what is does |
|
If i run the phpstan analysis locally everything is fine. Weird. |
|
That's because I've just added this rule into |
sabbelasichon
force-pushed
the
issue-6072
branch
from
d71971f
to
9d66685
Compare
|
Thank you 👍 Let's 🚢 it :) |
rectorphp/rector-src@f2d4be8 [FileSystem] Move filter <?= on last for files filter to make consistent with filter in directories for performance (#6073)
Resolves: #6072