feat: Clickable rule names in `TextReporter` pointing to official documentation #8003

llaville · 2024-05-11T04:45:53Z

Related to discussion #7996

…tHelpUri method

…property

…ary::changed property

coveralls · 2024-05-11T05:55:23Z

coverage: 96.071% (-0.05%) from 96.118%
when pulling 87734b4 on llaville:features/add_links_to_doc
into 3117b95 on PHP-CS-Fixer:master.

Wirone · 2024-05-11T05:59:45Z

src/Fixer/FixerInterface.php

+ * Returns the absolute URI [RFC3986](http://www.rfc-editor.org/info/rfc3986)
+ * of the primary documentation for the fixer.
+ */
+ public function getHelpUri(): string;


Since FixerInterface is not internal (it's a contract for custom fixers too) this is a breaking change. Is it required to get this info straight from the fixers, or could it be done using an external class?

I've recently released https://github.com/llaville/sarif-php-sdk/releases/tag/1.2.0 that will add SARIF support to most common PHP linters.

I've prepared a \Bartlett\Sarif\Converter\PhpCsFixerConverter for PHP-CS-Fixer too, that passed all validation by official Sarif Validator
But to be able to use it, the #7995 should be accepted ;-)

All, to said that SARIF used an helpUri property to enforce online documentation.

This is also my goal with PHP-CS-Fixer.

I understand that, but it does not change the fact it's a breaking change and requires major release, which we don't want now, as it would require massive amount of work to clean the codebase from deprecations ☺️. And it would require @keradus' review and approval, which often slows things down because of his limited availability. With non-invasive approach I think we can continue without him.

Having said that, we could make it part of a contract, but using composition - we can introduce a new interface, dedicated only for that, implement it internally to satisfy the reporter's needs, but at the same time let people implement it at their own pace. That would require checking if the fixer implements this interface while preparing the output and linking only these rules that provide a doc URL.

Agree with you to do it, but I'm sorry I won't have enough free time today !

Not a problem, everyone's busy. Let's continue as fast as possible, but without any rush ☺️.

@Wirone I'll work now on your idea to split interfaces to have something like :

<?php namespace PhpCsFixer\Fixer; interface FixerReportInterface { public function getHelpUri(): string; public function getExtraInfoFixers(): array; }

The getExtraInfoFixers will return any additional info as helpUri (e.g), but much more.

This interface will be usefull, in state only for TextReporter, because other Reporter don't provide online doc by format spec !

kubawerlos · 2024-05-11T09:24:45Z

src/Runner/Runner.php

@@ -176,6 +177,10 @@ private function fixFile(\SplFileInfo $file, LintingResultInterface $lintingResu
 $tokens->clearEmptyTokens();
 $tokens->clearChanged();
 $appliedFixers[] = $fixer->getName();
+
+ if (!isset($extraInfoFixers['helpUri'][$fixer->getName()])) {
+ $extraInfoFixers['helpUri'][$fixer->getName()] = $fixer->getHelpUri();


This is the only place getHelpUrl is called, so we can have the logic from AbstractFixer::getHelpUri in here and do not break BC, right?

@kubawerlos are you familiar with this comment? Because info about URIs would be used with another, dedicated reporter too.

But I did not verify it against that concept, so maybe you're right it would also work for it 🙂. Just wanted to make sure you have the context.

I saw it, and as in https://github.com/llaville/sarif-php-sdk/blob/1.2.0/src/Converter/PhpStanConverter.php the URLs are in there, not coming from PHPStan, so my understanding is that both PhpCsFixerConverter and Runner can have it (maybe shared by some new constant in PHP CS Fixer repo, but definitely not wich BC break change).

I'll give more explains later, especially in context of SARIF reports.
@kubawerlos The PhpCsFixerConverter is not yet available on my repo. Will come later when this PR will be finished !

Wirone · 2024-05-11T09:34:25Z

src/Console/Report/FixReport/ReportSummary.php

@@ -72,7 +72,7 @@ public function isDryRun(): bool
 }

 /**
- * @return array<string, array{appliedFixers: list<string>, diff: string}>
+ * @return array<string, array{appliedFixers: list<string>, diff: string, extraInfoFixers: array{helpUri: array<string, string>}|array{}}>


Why not:

Suggested change

* @return array<string, array{appliedFixers: list<string>, diff: string, extraInfoFixers: array{helpUri: array<string, string>}|array{}}>

* @return array<string, array{appliedFixers: list<string>, diff: string, helpUris: null|array<string, string>}>

or

Suggested change

* @return array<string, array{appliedFixers: list<string>, diff: string, extraInfoFixers: array{helpUri: array<string, string>}|array{}}>

* @return array<string, array{appliedFixers: list<string>, diff: string, helpUris?: array<string, string>}>

The first one make the helpUri nullable, the second one makes it optional. In both cases we can detect if there's info with URIs or not. I also find extraInfoFixers superfluous, I would rather add one level field like helpUris (plural, as it's a collection).

In fact, I would like to do appliedFixers: list<array{name: string, helpUri: string}>, but Runner also is not internal so we can't change the format of the returned array. Adding new field should be OK, though.

@keradus please let me know what you think 🙂.

@Wirone Stop wasting your time with this current PR. Even if code is almost ready, I should change the extraInfoFixers.

I'll give more explains when my new version will be ready. I hope to propose something in next hours (now, I'm back)

llaville · 2024-05-11T15:49:36Z

Now, after applying locally changes to initial PR proposal, and checks that all match my goals, I'll try to give you my best explains ;-)

From @Wirone comment about FixerInterface related to BC breaks, I've changed initial code and removed getHelpUri from contract.

Now, there are :

a new contract

<?php
namespace PhpCsFixer\Fixer;

interface FixerReportInterface
{
    public function getHelpUri(): string;

    public function getExtraInfoFixers(): array;
}

This contract is implemented by default to AbstractFixer to give benefit to all fixers,
and don't have BC breaks with custom fixers (that didn't extends it)

<?php
abstract class AbstractFixer implements FixerInterface, FixerReportInterface
{
    use FixerReportTrait;

}

Default implementation is given by trait

<?php
namespace PhpCsFixer\Fixer;

use PhpCsFixer\Utils;

trait FixerReportTrait
{
    public function getHelpUri(): string
    {
        $nameParts = explode('\\', static::class);
        $group = $nameParts[\count($nameParts) - 2];
        $name = substr(end($nameParts), 0, -\strlen('Fixer'));

        return sprintf(
            'https://cs.symfony.com/doc/rules/%s/%s.html',
            Utils::camelCaseToUnderscore($group),
            Utils::camelCaseToUnderscore($name)
        );
    }

    public function getExtraInfoFixers(): array
    {
        return [
            'helpUri' => $this->getHelpUri(),
            'descriptions' => [
                'short' => $this->getDefinition()->getSummary(),
                'full' => $this->getDefinition()->getDescription(),
                'risky' => $this->getDefinition()->getRiskyDescription() ?? $this->isRisky(),
            ],
        ];
    }
}

NB:

we may change contents (keys) of getExtraInfoFixers as we want with few impacts to Reporter(s)
I've added additional info (descriptions) that my PhpCsFixerReporter required to work well. Especially to produce SARIF ReportingDescriptor object that reflect the rules/fixers used.

E.g: chunk of result generated by my PhpCsFixerConverter class

                    "rules": [
                        {
                            "id": "phpdoc_no_alias_tag",
                            "name": "PhpdocNoAliasTag",
                            "shortDescription": {
                                "text": "No alias PHPDoc tags should be used."
                            },
                            "fullDescription": {
                                "text": "There is no description of Fixer and benefit of using it"
                            },
                            "defaultConfiguration": {
                                "enabled": true,
                                "level": "warning",
                                "rank": 1.0
                            },
                            "helpUri": "https://cs.symfony.com/doc/rules/phpdoc/phpdoc_no_alias_tag.html",
                            "help": {
                                "text": "https://cs.symfony.com/"
                            },
                            "properties": {
                                "frequency": 1,
                                "risky": false
                            }
                        }
                    ]

@Wirone If you're agree with these changes, I can push it to update the PR !

Let me know, if something is not enough clear ?

llaville · 2024-05-11T18:02:53Z

I've found a better fix/improvement, following https://phpstan.org/writing-php-code/phpdoc-types#array-shapes

array{"helpUri"?: string, "definition"?: FixerDefinitionInterface, "risky"?: bool}

That will cover all needs. Only "helpUri" is a new info/feature, "definition" and "risky" source already exists.

<?php

namespace PhpCsFixer\Fixer;

use PhpCsFixer\FixerDefinition\FixerDefinitionInterface;

interface FixerReportInterface
{
    /**
     * @return array{"helpUri"?: string, "definition"?: FixerDefinitionInterface, "risky"?: bool}
     */
    public function getExtraInfoFixers(): array;
}

And default implementation could be directly included into AbstractFixer rather than create a trait (that is used nowhere else)

<?php 

namespace PhpCsFixer;

abstract class AbstractFixer implements FixerInterface, FixerReportInterface

{
    protected function getHelpUri(): string
    {
        $nameParts = explode('\\', static::class);
        $group = $nameParts[\count($nameParts) - 2];
        $name = substr(end($nameParts), 0, -\strlen('Fixer'));

        return sprintf(
            'https://cs.symfony.com/doc/rules/%s/%s.html',
            Utils::camelCaseToUnderscore($group),
            Utils::camelCaseToUnderscore($name)
        );
    }

    public function getExtraInfoFixers(): array
    {
        return [
            'helpUri' => $this->getHelpUri(),
            'definition' => $this->getDefinition(),
            'risky' => $this->isRisky(),
        ];
    }

}

And extraInfoFixers data is populated from Runner

<?php
namespace PhpCsFixer\Runner;

final class Runner
{ 

    private function fixFile(\SplFileInfo $file, LintingResultInterface $lintingResult): ?array
    {

                if ($tokens->isChanged()) {
                    $tokens->clearEmptyTokens();
                    $tokens->clearChanged();
                    $appliedFixers[] = $fixer->getName();

                    if ($fixer instanceof FixerReportInterface) {
                        $extraInfoFixers[$fixer->getName()] = $fixer->getExtraInfoFixers();
                    }
                }

        if ($oldHash !== $newHash) {
            $fixInfo = [
                'appliedFixers' => $appliedFixers,
                'diff' => $this->differ->diff($old, $new, $file),
                'extraInfoFixers' => $extraInfoFixers,
            ];

        }

    }

}

llaville · 2024-05-12T06:28:21Z

@Wirone, @kubawerlos I've pushed on my repo (branch 1.3) a little tuto that demonstrate https://github.com/llaville/sarif-php-sdk/tree/1.3/examples/converters/phpcs-fixer how we can see TextReporter with clickable links and generate a SARIF output report.

Following the same principle in action since https://github.com/llaville/sarif-php-sdk/releases/tag/1.2.0 for PHPCS, PHPLint, and PHPStan SARIF converters

keradus · 2024-05-12T09:16:03Z

src/AbstractFixer.php

+ 'definition' => $this->getDefinition(),
+ 'risky' => $this->isRisky(),


huh? no proxy methods please
we already have isRisky() and getDefinition(), no need for method that is duplicating this knowledge

If you don't want to have external SARIF output format to your project I understand. Otherwise, consider that, as helpUri the extra info that missed from ReportSummary class, to generate full (validated) SARIF results which identify complety all rules/fixers applied !

I know that is not in context of this PR, but as we try to add an additional info (helpUri) here I took advange of the situation.

I believe we should focus on minimal changes required for achieving clickable URLs in the output - nothing more, nothing less. That would make this PR much easier to process and finish. Then, it could be extender or/and refactored to achieve SARIF reporting - for now I don't even have the context what it is, what it needs and I don't want to dig into that at this point. Having said that, I agree with @keradus' comment, and I would prefer first option from this comment. Let's don't model stuff for things that will or won't land later.

That's why I don't like FixerReportInterface because we don't need to focus here on reporting (clickable URL can be also added to describe CLI command), I would prefer something more atomic, focused on docs URI. ProvidesDocumentationUriInterface or something like that, that would allow adding only this particular feature to fixers.

FYI: If you wan't to know what is SARIF, consider to take time to read this page https://docs.github.com/en/code-security/code-scanning/integrating-with-code-scanning/sarif-support-for-code-scanning.
It's a good intro !

keradus · 2024-05-12T09:17:57Z

src/Console/Report/FixReport/TextReporter.php

@@ -41,6 +43,7 @@ public function generate(ReportSummary $reportSummary): string
 $output .= $this->getAppliedFixers(
 $reportSummary->isDecoratedOutput(),
 $fixResult['appliedFixers'],
+ $fixResult['extraInfoFixers']


I really wonder if we need to change the fixResult object, or simply have one standalone method that is converting fixer-name to fixer-url

I will let you think again then ...

keradus

I like the idea, but we overkill the implementation imho

llaville · 2024-05-12T14:00:13Z

I like the idea, but we overkill the implementation imho

As all project leaders, you're the last one to choose.

The first approach is to consider this PR to only add feature to click on rule names to go to official documentation.
The alternative approach is to consider to re-work the ReportSummary class that have not enough information to produce additional report format (like SARIF).
The last approach I can consider is to close the report formats list (in this case we can close the Add ability to register custom reporters #7995) and don't provide additional Reporter classes, and of course the definition and risky proxy info I've consider to add to extraInfoFixers are not necessary.

Please let me know your answer, and I will heard it.

llaville · 2024-05-12T15:30:37Z

FYI: it works fine with custom fixers like https://github.com/kubawerlos/php-cs-fixer-custom-fixers

Fixers that don't implement new Interface (whatever is name should be) helpUri extraInfo is not provided so rule name link is not clickable.

llaville · 2024-05-13T07:20:55Z

Closing this PR.
Waiting your answer about new kind of implementation (new Interface name if any), where to implement the code to retrieve help URI ... and whatever else you've in mind

Wirone · 2024-05-13T08:35:57Z

There's no need to close the PR though, everyone works at their own pace and I just did not have the possibility to look at this more during the weekend. I already provided info on how I see it, so the scope of this PR could be narrowed to a simple 1-method interface, with implementation focused only on the clickable rule name. The name of the interface could be then discussed. I don't want to point every bit of the code (what and where), this way I would rather prefer to do the implementation myself ☺️.

llaville added 7 commits May 11, 2024 03:09

add ability to show the primary documentation of fixers applied

773b0b0

follows @Wirone recommendation about href link on discussion PHP-CS-F…

c846f61

…ixer#7996

fix existing unit tests about new PhpCsFixer\Fixer\FixerInterface::ge…

d0920ee

…tHelpUri method

follows @Wirone recommendation about removing verbosity level 2 on di…

b0516bd

…scussion PHP-CS-Fixer#7996

fix phpdoc about new entry extraInfoFixers of ReportSummary::changed …

3ccb518

…property

fix existing unit tests about new entry extraInfoFixers of ReportSumm…

8919afb

…ary::changed property

fix phpdoc cs

175e8a2

Wirone changed the title ~~Add support to link on official documentation to TextReporter~~ feat: Clickable rule names in TextReporter pointing to official documentation May 11, 2024

fix RunnerTest with missing extraInfoFixers/helpUri entry

48b82ca

Wirone requested changes May 11, 2024

View reviewed changes

kubawerlos reviewed May 11, 2024

View reviewed changes

Wirone reviewed May 11, 2024

View reviewed changes

llaville added 3 commits May 11, 2024 19:00

introduces FixerReportInterface to avoid BC breaks with custom fixers

0199bab

fix RunnerTest

18091f5

fix regression

87734b4

llaville requested a review from Wirone May 11, 2024 20:23

keradus reviewed May 12, 2024

View reviewed changes

keradus requested changes May 12, 2024

View reviewed changes

llaville closed this May 13, 2024

llaville deleted the features/add_links_to_doc branch May 13, 2024 07:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Clickable rule names in `TextReporter` pointing to official documentation #8003

feat: Clickable rule names in `TextReporter` pointing to official documentation #8003

llaville commented May 11, 2024

coveralls commented May 11, 2024 •

edited

Wirone May 11, 2024

llaville May 11, 2024

Wirone May 11, 2024

llaville May 11, 2024

Wirone May 11, 2024 •

edited

llaville May 11, 2024

kubawerlos May 11, 2024

Wirone May 11, 2024

Wirone May 11, 2024

kubawerlos May 11, 2024

llaville May 11, 2024

Wirone May 11, 2024

llaville May 11, 2024

llaville commented May 11, 2024

llaville commented May 11, 2024

llaville commented May 12, 2024

keradus May 12, 2024

llaville May 12, 2024

Wirone May 12, 2024

llaville May 12, 2024

keradus May 12, 2024

llaville May 12, 2024

keradus left a comment

llaville commented May 12, 2024

llaville commented May 12, 2024

llaville commented May 13, 2024

Wirone commented May 13, 2024

	* @return array<string, array{appliedFixers: list<string>, diff: string, extraInfoFixers: array{helpUri: array<string, string>}\|array{}}>
	* @return array<string, array{appliedFixers: list<string>, diff: string, helpUris: null\|array<string, string>}>

		'definition' => $this->getDefinition(),
		'risky' => $this->isRisky(),

feat: Clickable rule names in TextReporter pointing to official documentation #8003

feat: Clickable rule names in TextReporter pointing to official documentation #8003

Conversation

llaville commented May 11, 2024

coveralls commented May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Wirone May 11, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

llaville commented May 11, 2024

llaville commented May 11, 2024

llaville commented May 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

keradus left a comment

Choose a reason for hiding this comment

llaville commented May 12, 2024

llaville commented May 12, 2024

llaville commented May 13, 2024

Wirone commented May 13, 2024

feat: Clickable rule names in `TextReporter` pointing to official documentation #8003

feat: Clickable rule names in `TextReporter` pointing to official documentation #8003

coveralls commented May 11, 2024 •

edited

Wirone May 11, 2024 •

edited