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

Fix lint in activate.ps1 #1371

Closed
jayvdb opened this issue Jun 21, 2019 · 12 comments

Comments

@jayvdb
Copy link
Contributor

commented Jun 21, 2019

PSScriptAnalyzer is the defacto standard PS linter, and it isnt very good at ignoring files. c.f. PowerShell/PSScriptAnalyzer#1230

As a result, a few times I have had activate.ps1 appear in my results in CI, and it is usually rather hard to get eliminate those results. I'm now wrapping the linter and post-processing the results, but a better solution is to fix the problem at the source. The lint warnings are quite reasonable.

RuleName                            Severity     ScriptName Line  Message                                                     
--------                            --------     ---------- ----  -------                                                     
PSUseDeclaredVarsMoreThanAssignment Warning      activate.p 22    The variable 'old_env' is assigned but never used.          
s                                                s1                                                                           
PSAvoidUsingWriteHost               Warning      activate.p 59    File 'activate.ps1' uses Write-Host. Avoid using Write-Host 
                                                 s1               because it might not work in all hosts, does not work when  
                                                                  there is no host, and (prior to PS 5.0) cannot be           
                                                                  suppressed, captured, or redirected. Instead, use           
                                                                  Write-Output, Write-Verbose, or Write-Information.          
PSAvoidUsingWriteHost               Warning      activate.p 68    File 'activate.ps1' uses Write-Host. Avoid using Write-Host 
                                                 s1               because it might not work in all hosts, does not work when  
                                                                  there is no host, and (prior to PS 5.0) cannot be           
                                                                  suppressed, captured, or redirected. Instead, use           
                                                                  Write-Output, Write-Verbose, or Write-Information.          
PSAvoidGlobalVars                   Warning      activate.p 45    Found global variable 'global:_OLD_VIRTUAL_PATH'.           
                                                 s1                               

Also I notice that activate.ps1 has been switching between various brackets styles. PSScriptAnalyzer also has a code formatter built in, with a few preset styles available. The formatter is a bit trickier to invoke as it processes a code string rather than recursing through a file path, but that shouldnt be a problem here because there is only one .ps1 file in this repo. It it can also dedent everything incorrectly if the code didnt have sufficient blank lines or indentation at the right spots -- if that happens, reset the file, format the code a little better, and then it will Do The Right Thing.

A simple helper can be added to lint this file as part of the Windows CI, to ensure there are no lint or style problems.

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

Thanks for taking a look at the activate.ps1 file and filing this @jayvdb! :D

The lint warnings are quite reasonable.

Indeed. I think it'll be great if someone familiar with PowerShell would file a PR, fixing these lint warnings and more consistently styling the script. :)

A simple helper can be added to lint this file as part of the Windows CI, to ensure there are no lint or style problems.

I'm ambivalent on this since I don't expect this file to change often. I defer to others on this.

@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Probably useful to get opinions of the previous authors, to see if any particular style is preferred. If none, the best choice would be the PSScriptAnalzyer default (1TBS), which also happens to be the required setting for a nice helper https://github.com/sqlcollaborative/dbatools/blob/4f424b72b394a7c6b8fa7918768daad134ae7db8/functions/Invoke-DbatoolsFormatter.ps1 which happens to have been broken in the latest release sqlcollaborative/dbatools#5800)

@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 21, 2019

Pinging for any strong powershell style preferences @bskinn, @gaborbernat, @ferdinandvanwyk, @pfmoore, and also @regisf due to https://github.com/regisf/virtualenvwrapper-powershell , though I am not sure that is the same as http://code.philsturgeon.co.uk/guillermooo/virtualenvwrapper-powershell/overview mentioned in 1084103

If there are no strong opinions, I'll go ahead with PSScriptAnalzyer default 1TBS style

@bskinn

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2019

No style prefs at all here, I'm all for standardized/automatic formatting!

And, if a linter would be easy to set up, I don't see why not to do so. The Python code is linted, so why not also lint the scripts?

Are there any linters for the other supported non-Python shells (cmd/bash/fish/csh)?

@pradyunsg

This comment has been minimized.

Copy link
Member

commented Jun 21, 2019

jayvdb added a commit to jayvdb/virtualenv that referenced this issue Jun 25, 2019

Lint activate.ps1
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormattingOTBS

Closes pypa#1371

jayvdb added a commit to jayvdb/virtualenv that referenced this issue Jun 25, 2019

Lint activate.ps1
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormattingOTBS

Closes pypa#1371
@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

I dug around a bit more and the default style rules appear to be CodeFormatting, which is slightly different to their 1TBS style. I havent analyzed the differences yet.

I do have confirmation from the maintainers that PSScriptAnalyzer needs to be run twice to get both syntax (std) and style errors (using one of the above settings files) https://gitter.im/PowerShell/PSScriptAnalyzer?at=5d12f28a11ba925f6a253968 But it might be possible to create our own settings .psd1 file which explicitly contains both sets of rules. I think two invocations using defaults is better than adding a custom .psd1 that needs to be maintained.

@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jun 26, 2019

The main difference I can find is that OTBS requires } else { while without OTBS it needs to be }\n(indent)else {

@bskinn

This comment has been minimized.

Copy link
Contributor

commented Jun 26, 2019

Definitely agree a double-run with defaults is far preferable. Should be as simple as > (pssa --syntax) -and (pssa --style)?

I personally like the } else { style, but it's definitely idiosyncratic compared to... well, just about everything else.

jayvdb added a commit to jayvdb/virtualenv that referenced this issue Jun 27, 2019

Lint activate.ps1
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormattingOTBS

Closes pypa#1371

jayvdb added a commit to jayvdb/virtualenv that referenced this issue Jul 21, 2019

Lint activate.ps1
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormattingOTBS

Closes pypa#1371
@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2019

Should be as simple as > (pssa --syntax) -and (pssa --style)?

@bskinn , I ended up down a rabbit hole there and eventually found their builtin style formatting style definitions are incompatible with the linter PowerShell/PSScriptAnalyzer#769

The difference doesnt make much difference in this repo, but for any significant chunk of code, a custom .psd1 is needed. https://github.com/coala/coala-bears/blob/master/.ci/PSScriptAnalyzerSettings.psd1 is the one I used to reformat activate.ps1 , and I have a hacky linter script https://github.com/coala/coala-bears/blob/master/.ci/PSLint.ps1 to run the pssa linter and formatter and keep the file in the repo in Unix EOL.

activate.ps1 is currently using DOS EOL. I suggest that should also be changed. I could do that in my PR if desired, as a second commit so it is easy to see that the DOS->Unix EOL commit did "nothing".

jayvdb added a commit to jayvdb/virtualenv that referenced this issue Jul 21, 2019

Lint activate.ps1
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormatting

Closes pypa#1371
@bskinn

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2019

Man, sounds like it was quite a pain. Thanks for investigating & getting the infrastructure in place!

Why are the DOS EOLs an issue? Do they choke the linter?

Currently, AFAIK all of the tests are set such that cmd/posh tests are skipped on non-Windows systems. If the ps1 linter hacks a furball on Linux VMs, then how about let's just add a trap in the CI to skip the linting on non-Windows?

@gaborbernat, thoughts?

@jayvdb

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2019

DOS EOLs are merely annoying, but were not causing any problems.

I suggest using Unix EOLs only so that the repo has more consistent EOLs. I'm not a frequent Windows user/developer, so I dont know if there are bugs caused by using Unix EOLs for .ps1 or .cmd, but it seems most Windows tools can handle Unix EOLs now.

gaborbernat added a commit that referenced this issue Jul 23, 2019

Lint activate.ps1 (#1373)
Fix PSScriptAnalyzer warnings, and follow style from
-Setting CodeFormatting

Closes #1371
@bskinn

This comment has been minimized.

Copy link
Contributor

commented Jul 23, 2019

I would be hesitant to convert activate.bat (especially) or activate.ps1 to Unix EOLs at this point. In the past, Unix EOLs have caused me mysterious and ~random problems at times...I'm pretty sure cmd syntax is (or has been) EOL-sensitive, at least.

Probably better to give these recent improvements to cms/posh time to penetrate before attempting an EOL conversion, IMO. And, even then, a quick reversion might be needed if things blow up on older systems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.