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

Excluded path: include example for optional path #3883

Merged
merged 4 commits into from
Mar 19, 2025

Conversation

thePanz
Copy link
Contributor

@thePanz thePanz commented Mar 18, 2025

Make more clear how to mark an excluded path optional, with the use of (?) suffix

@@ -380,7 +380,8 @@ public static function begin(
}

$errorOutput->writeLineFormatted('If the excluded path can sometimes exist, append <fg=cyan>(?)</>');
$errorOutput->writeLineFormatted('to its config entry to mark it as optional.');
$errorOutput->writeLineFormatted('to its config entry to mark it as optional. Example:');
$errorOutput->writeLineFormatted(' <fg=cyan>- path/something (?)</>');
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to show the full example including excludePaths: and to use the actual path in question, not just path/something. Thanks.

Copy link
Contributor Author

@thePanz thePanz Mar 18, 2025

Choose a reason for hiding this comment

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

I tried to achieve that as my first contribution to PHPStan :)
I proposed a first change to be able to track which paths should be suggested.

from my understanding: paths adhering to FileExcluder::isFnmatchPattern() are not evaluated, thus I opted to not suggest those as Optional. Let me know if those should be added too as suggestions.

@thePanz thePanz force-pushed the patch-1 branch 4 times, most recently from 8137302 to dad73e9 Compare March 18, 2025 23:59
Copy link
Member

@ondrejmirtes ondrejmirtes left a comment

Choose a reason for hiding this comment

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

I've just added a single entry that does not exist like this:

parameters:
	excludePaths:
		- foo

But your code results in the following output:

Screenshot 2025-03-19 at 9 02 25

Please fix that.

@ondrejmirtes
Copy link
Member

Also I just noticed there's a typo: analyseAndScan is missing a colon.

@thePanz thePanz marked this pull request as draft March 19, 2025 08:47
@gharlan
Copy link
Contributor

gharlan commented Mar 19, 2025

The example should use relative paths.

@ondrejmirtes
Copy link
Member

It should but right now it's impossible to get the correct relative path.

Because relative path depends on the location of the config file, but at this point we have the whole parameters array decoded from all the config files at once.

I don't need OP to tackle this. I can look at it myself once the PR works.

@thePanz
Copy link
Contributor Author

thePanz commented Mar 19, 2025

Some brain dump while trying to implement the feature:

  1. in ValidateExcludePathsExtension::loadConfiguration() we get the paths resolved and absolute, we would need a way to figure out the original value passed in the configuration
  2. the $this->getContainerBuilder() returns what I call "fully qualified configuration" where the shortcut mentioned here https://phpstan.org/user-guide/ignoring-errors#excluding-whole-file is already resolved to the full configuration

@thePanz
Copy link
Contributor Author

thePanz commented Mar 19, 2025

Got this result:
image

By using the following configuration:

includes: [ build/phpstan.neon ]
parameters:
	excludePaths:
		- foo

@ondrejmirtes
Copy link
Member

Please mark it as ready for review if it's done from your side.

@thePanz thePanz marked this pull request as ready for review March 19, 2025 10:29
@phpstan-bot
Copy link
Collaborator

This pull request has been marked as ready for review.

thePanz and others added 4 commits March 19, 2025 11:35
Make more clear how to mark an excluded path optional, with the use of `(?)` suffix
…t configuration
@ondrejmirtes
Copy link
Member

I've pushed two commits, feel free to inspect them 😊

@ondrejmirtes ondrejmirtes merged commit 85c709d into phpstan:2.1.x Mar 19, 2025
100 of 104 checks passed
@ondrejmirtes
Copy link
Member

Thank you!

@thePanz thePanz deleted the patch-1 branch March 20, 2025 08:15
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.

None yet

4 participants