Skip to content

Allow relative settings path #909

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

Merged

Conversation

bergmeister
Copy link
Collaborator

PR Summary

Fixes #908 to make it possible to specify a relative path for the -Settings paramater.

PR Checklist

Note: Tick the boxes below that apply to this pull request by putting an x between the square brackets. Please mark anything not applicable to this PR NA.

  • PR has a meaningful title
    • Use the present tense and imperative mood when describing your changes
  • Summarized changes
  • [NA] User facing documentation needed
  • Change is not breaking
  • Make sure you've added a new test if existing tests do not effectively test the code changed
  • This PR is ready to merge and is not work in progress
    • If the PR is work in progress, please add the prefix WIP: to the beginning of the title and remove the prefix when the PR is ready

Copy link
Contributor

@JamesWTruher JamesWTruher left a comment

Choose a reason for hiding this comment

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

I'm a little concerned with the approach. PS can actually resolve the path before it is handed off to the lower level routines. What do you think of the following? (line 296 or so in InvokeScriptAnalyzerCommand.cs)

                try {
                    // Attempt to convert the settings into a path
                    ProviderInfo resolvedProvider = null;
                    var resolvedSettingPaths = GetResolvedProviderPathFromPSPath(settings.ToString(), out resolvedProvider);
                    if ( resolvedSettingPaths != null ) {
                        settings = resolvedSettingPaths.FirstOrDefault<string>();                    
                    }
                }
                catch { ; }

FWIW, there are alot of cmdlets which do something similar. Then you can drop changes in Engine/Settings.cs.

@bergmeister
Copy link
Collaborator Author

bergmeister commented Feb 26, 2018

Thanks for the feedback, I did not know that there is a neater way pf resolving the path and in general I would approve doing that.
What made the change difficult here is because the settings object can either be a string (path) or a hashtable of settings and there is a lot of weird logic/dependencies going on later in the Settings class being contructed by PSSASettings.Create (e.g. only later on it will determine the type but then we do not have access to the methods of PSCmdlet any more). The downside of your approach is that we need to have a catch then in case a hashtable gets passed as a setting and using exceptions for control flow is generally discouraged in .Net for performance reasons. I know that one should not micro-optimize but this change would hit every single user of the powershell vscose extension. What do you think? Should we then maybe rather pass the resolver to the Settings class instead as a compromise?

@JamesWTruher
Copy link
Contributor

I noticed that clever business with regard to the hashtable vs. string, but I put it in the cmdlet because that's traditionally where resolution code resides (rather than the engine). I suppose it could be put in the actual setter of the parameter and in the case that it's given a string, only then attempt the path resolution, or moved to the engine for that matter. I don't have a problem with a different location for the resolution, I was just following a different pattern.
One nice side-effect of using the path resolution API is that you get wildcard support for free, so the user could provide something like:
Invoke-ScriptAnalyzer -Setting "Settings/*/Gallery.psd1
which might be handy

@bergmeister
Copy link
Collaborator Author

bergmeister commented Feb 27, 2018

Using some delegate trickery (due to the resolver method having an out parameter), I am now passing the resolver method pointer to the settings class so that only it only gets used once the argument type is determined to be of type File. This also revealed that the Invoke-Formatter must have suffered from the same bug and solves this one now as well. Any exceptions are still caught on a higher level, therefore if anything goes wrong, the call to .Single() will make sure an exception gets raised.
I follow now also the rule to update the licence header in every file that I touch to the new recommended on in the PSCore repo and clean up using statements.

@JamesWTruher
Copy link
Contributor

looks great

@JamesWTruher JamesWTruher merged commit d4ba947 into PowerShell:development Feb 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow relative path in -Settings parameter.
2 participants