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

Include identifier in generated baseline #844

Closed
wants to merge 1 commit into from

Conversation

ruudk
Copy link
Contributor

@ruudk ruudk commented Dec 12, 2021

This makes it possible to analyse the baseline and manually group common errors by identifier.

In the future, the logic can be used to remove the message patterns and only ignore on identifier.

That can help reduce the file size in large legacy projects.

References:
phpstan/phpstan#1686 (comment)
phpstan/phpstan#3065

This makes it possible to analyse the baseline and manually group common errors by `identifier`.

In the future, the logic can be used to remove the message patterns and only ignore on identifier.

That can help reduce the file size in large legacy projects.

References:
phpstan/phpstan#1686 (comment)
phpstan/phpstan#3065
@ondrejmirtes
Copy link
Member

Hi, thanks for your contribution, but it's useless to add something to the baseline that's not used in the actual ignoring 😊

If you want to know about identifiers, feel free to use a custom error formatter. But they're really rarely reported right now. Once all the rules have them, they're gonna be used in more places (like ignoring and in configuration).

@ruudk ruudk deleted the identifier branch December 12, 2021 19:28
@ruudk
Copy link
Contributor Author

ruudk commented Dec 12, 2021

I notice that a lot of the errors don't have the identifier yet. It's a bit chicken and egg problem.

What if we start using the identifier to ignore errors when it's present? That could push the migration to add more identifiers.

I'm already trying that here for example:

@ondrejmirtes
Copy link
Member

Identifier alone isn't precise enough for the baseline.

@ruudk
Copy link
Contributor Author

ruudk commented Dec 13, 2021

I found this:

->identifier('deadCode.elseifConstantCondition')
->metadata([
'depth' => $node->getAttribute('statementDepth'),
'order' => $node->getAttribute('statementOrder'),
'value' => $exprType->getValue(),
])

Does that mean that in the future, the ignoring (baseline) will be done based on identifier + metadata? Is that the vision?

@ondrejmirtes
Copy link
Member

Yes, that's the idea :)

@ruudk
Copy link
Contributor Author

ruudk commented Dec 14, 2021

Can I give it a shot?

Use identifier and metadata when available and write it to baseline when a feature flag is enabled (can be part of bleeding edge). Errors with identifier and metadata won't write a message pattern.

When filtering errors that are in the baseline, check if the baseline item has identifier/metadata and compare that to RuleError. Also support filtering by message (when baseline is in old/current format).

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

Successfully merging this pull request may close these issues.

2 participants