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

Add explicit resolveSymlinks option for project directories #7163

Merged
merged 3 commits into from Jan 10, 2022

Conversation

dvz
Copy link
Contributor

@dvz dvz commented Dec 14, 2021

Partially resolves .#7057

A candidate approach that involves skipping the expensive recursive scanning of project directories (added via 07b29e4) unless enabled for the individual directory (usually the vendor directory, according to #1141).

E.g.:

<ignoreFiles>
    <directory name=".git" />
    <directory name="other-big-subdirectory" />
    <directory name="vendor" resolveSymlinks="true" />
</ignoreFiles>

@orklah
Copy link
Collaborator

orklah commented Dec 15, 2021

So, for a user that actually uses this composer feature, this would break the analysis because Psalm would not be able to find vendors anymore, correct?

If that's so, we may want to default this to false first. If we can detect the symlink somehow, it would be great to emit a deprecation to warn about the config, and then we switch the config default in Psalm 5

@dvz
Copy link
Contributor Author

dvz commented Dec 15, 2021

If only ignored files are affected (as claimed in the original issue - i.e. Psalm can resolve symlinks on its own using realpath(), but those final paths won't be covered by the configured ignore list), it's possible that this will result in symlinked paths being unintentionally scanned, but wouldn't reduce coverage nor result in errors.

Otherwise, adding a global option (and passing them to FileFilter) may be necessary.

@orklah
Copy link
Collaborator

orklah commented Dec 21, 2021

Merging your other PR created a conflict here. Mind rebasing?

@orklah orklah added the release:internal The PR will be included in 'Internal changes' section of the release notes label Jan 10, 2022
@orklah
Copy link
Collaborator

orklah commented Jan 10, 2022

Thanks!

@orklah orklah merged commit d52c3f0 into vimeo:master Jan 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release:internal The PR will be included in 'Internal changes' section of the release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants