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

[ci] check PowerShell scripts with PSScriptAnalyzer #6700

Closed
wants to merge 33 commits into from

Conversation

StrikerRUS
Copy link
Collaborator

@StrikerRUS StrikerRUS commented Oct 27, 2024

Fixed #6498.
Fix reported warnings from #6498 (comment) and new ones from non-default rules.

@StrikerRUS
Copy link
Collaborator Author

This PR looks quite big for reviewing. I think it's better to split it into several smaller PRs. @jameslamb WDYT?

@jameslamb
Copy link
Collaborator

I agree @StrikerRUS , I think it'd be better as separate PRs. How about just 2?

First PR:

  • whitespace / indentation changes (including to other files like lint-r-code.R)
  • .editorconfig changes
  • installing and running ScriptAnalyzer (but with a || exit 0, so it echoes to logs but isn't fully enforced)
  • Check-Output to Assert-Output change

Second PR:

  • any remaining changes
  • switching that || exit 0 to || exit 1 to enforce it

If that's too difficult, then I'm happy to follow any sequence you want and I'll try to provide reviews quickly. But at a minimum I think the first PR should introduce the test.sh changes with || exit 0, similar to what we did in the past for cpplint (#1990).

@StrikerRUS
Copy link
Collaborator Author

@jameslamb Yeah, sure! I'll split this PR into 2 or 3 smaller ones that will be comfortable to review.

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.

[ci] enforce 'shellcheck' checks
2 participants