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

Lint: PSAvoidUsingCmdletAliases #2075

Merged

Conversation

chawyehsu
Copy link
Member

Replace all cmdlet aliases with their full content.

@r15ch13
Copy link
Member

r15ch13 commented Mar 2, 2018

Nice! Is there a way to enforce it by adding it to the tests?

@chawyehsu
Copy link
Member Author

@r15ch13
I think we should lint the whole project for future mantainance, PSAvoidUsingCmdletAliases is the first step of this big task.

The official PowerShell script linter is the PSScriptAnalyzer, currently we could use it through:

  1. Install From PowerShell Gallery, then use it through console.
  2. Install VSCode PowerShell extension, then use it in VSCode.

But the PSScriptAnalyzer is still problematic, some rules are false positive. e.g. PSUseDeclaredVarsMoreThanAssignment.

It seems that PowerShell script linting toolchain remain to be perfected, but of course, the automatic linting and tests should be improved. Here I just give a prompt to set the ball rolling. 😂

@rasa
Copy link
Member

rasa commented Mar 3, 2018

These changes make the code much easier to read and maintain. Can we add a lint check to our test suite, and/or build chain?

@bergmeister
Copy link

bergmeister commented Mar 3, 2018

Just a heads up (I am the community maintainer of PSScriptAnalyzer ): The next upcoming release of PSSA will have a -Fix switch that can autofix you warnings like e.g. PSAvoidUsingCmdletAliases. It is good that you removed them, the PowerShell team is planning to take out aliases in one of the next versions (but one will still be able to use them by installing an optional module) because they make so many problems (mainly due to clashes with other binaries in the Windows and Linux world).
We are aware that PSUseDeclaredVarsMoreThanAssignment has many false positives. Some enhancements will come in the next version but this rule is a tricky one (even from a theoretical point of view, it is difficult to assess when a variable is being used and not in terms of scoping). But we are open to PRs :-)

@r15ch13 r15ch13 merged commit e1bb1e9 into ScoopInstaller:master Mar 3, 2018
@chawyehsu chawyehsu deleted the lint-ps-avoid-using-cmdlet-aliases branch March 10, 2018 03:24
@excitoon
Copy link
Contributor

excitoon commented Mar 11, 2018

Luke Sampson has proudly ignored Powershell Verb-Noun naming conventions. https://github.com/lukesampson/psutils/blob/master/README.md
Well, that really improves readability.

r15ch13 pushed a commit that referenced this pull request Mar 14, 2018
* Fix interpretation of response's status code to detect redirections

* Improve documentation of virustotal subcommand

- usage & configuration of virustotal_api_key
- special parameter '*' to test all installed apps
- make necessity of having a virustotal_api_key for --scan explicit
- show that it's possible to check several packages at once

* Never use virustotal_api_key to query if a package is safe

The URL in the code wasn't an API end-point anyway.

* Refactor logic to warn user about apps unknown to VirusTotal

* Warn once when virustotal_api_key's absence prevents VirusTotal submission

This is preparation for changes to come in the package submission logic.

* Use API to submit download link to VirusTotal, rate limited in EAFP fashion

This is a roundabout way to get the file to be scanned without having
to download & upload it ourselves.

Rate limiting is implemented using EAFP: if submission fails, we wait
at least 60s before retrying at most once.

* Color undecided VirusTotal information the same way as `dangerous' files

If the scanning is still in progress, VirusTotal returns 0 malicious,
0 suspicious and 0 undetected.  Err on the safe side and color this
the same way as `dangerous' files.

* Remove requirement to only verify installed apps

The initial use case for this feature was to scan packages to avoid
installing dangerous apps.  Assuming they are infected, we want if
possible to avoid downloading them at all.

* Check dependencies with VirusTotal, too (by default)

* Manually apply `Lint: PSAvoidUsingCmdletAliases' (see e1bb1e9, #2075)

This is to avoid conflicts when merging lukesampson:master

* Explain applist's return value transformation: drop `global' flag for each app

* Move variable declarations and apps list generation to the top

* Reformat code and comply to linted function names

* Reduce nesting, remove hacky hash/url retrieval

* Remove $global variables

* Fix regression bug in Search-VirusTotal()

* Remove applist() because it's irrelevant if app is installed globally
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.

6 participants