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

Make PSSA look for a file in the workspace root by default #2484

Merged
merged 2 commits into from
Feb 20, 2020

Conversation

rjmholt
Copy link
Contributor

@rjmholt rjmholt commented Feb 20, 2020

PR Summary

Fixes #2190.

Copy link
Member

@TylerLeonhardt TylerLeonhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@rjmholt rjmholt merged commit b6fb0c4 into PowerShell:master Feb 20, 2020
@bergmeister
Copy link
Contributor

bergmeister commented Feb 24, 2020

@TylerLeonhardt @rjmholt
Have you guys actually tested this change?
Because I don't think this works at all because the logic a few layers underneath is something along the lines:
If the setting is not defined, then pass the default set of PSSA rules as settings as defined in PSES, if it is defined, pass that file path. There is no checking that the file actually exists and therefore with this default, you've just completely turned off PSSA warnings for 95% of the users, which are the ones not using a PSSA settings file... PSSA itself throws an error if the settings file does not exist but I mentioned previously that for some reason, PSES ignores or does not propagate PSSA errors.

@TylerLeonhardt
Copy link
Member

All of that logic you speak of was added in this PR:

PowerShell/PowerShellEditorServices#1179

@rjmholt
Copy link
Contributor Author

rjmholt commented Feb 24, 2020

@bergmeister are you saying that you’re experiencing the behaviour you’re describing?

@bergmeister
Copy link
Contributor

bergmeister commented Feb 24, 2020

First of all, mea culpa, I did not see the other recent PSES change, thanks for pointing me to it.
But at the same point I also pulled the latest changes from PSES and vscode-powershell from master into my forks before writing the comment and yes, I am experiencing the behaviour of PSSA warnings not showing up at all.

@rjmholt
Copy link
Contributor Author

rjmholt commented Feb 24, 2020

I’m assuming you don’t have a PSScriptAnalyzerSettings.psd1 defined in the root of the workspace where you’re seeing this? (Since that would result in behaviour like this — PSSA doesn’t have much in the way of error reporting on settings)

@bergmeister
Copy link
Contributor

bergmeister commented Feb 24, 2020

Yes, I don't have a settings file at the root (deliberately, to mirror what most people work with every day).
We cannot just disregard the most common use case and put the blame on PSSA, this is a big breaking change to the user experience.

A few months ago, I tried to do a prototype to auto-discover if a settings file exists by writing some typescript code in the vscode-powershell repo to check if a file named PSScriptAnalyzerSettings.psd1 exists (which did work well and which I suggest to be implemented) but the reason why I did not proceed further is because I couldn't change the code to use that discovered settings path (because of the global nature of the setting, I couldn't just pass the value to something on a lower level or change the setting value behind the scenes)

@TylerLeonhardt
Copy link
Member

@bergmeister it's a bug. Please understand we wouldn't break the customer like that. That would be silly.

@rjmholt
Copy link
Contributor Author

rjmholt commented Feb 24, 2020

Migrating this discussion to #2489

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.

Set the powerShell.scriptAnalysis.settingsFilePath setting to a default for auto-discoverability
3 participants