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

Require directoryOrFile argument when not piping stdin #388

Merged
merged 7 commits into from
Aug 15, 2021
Merged

Conversation

belav
Copy link
Owner

@belav belav commented Aug 3, 2021

This also includes creating a number of tests that run with a github workflow to ensure we don't introduce bugs into the CLI with any later changes.
closes #381

@belav belav force-pushed the fix-check branch 19 times, most recently from 3462bbb to e821b76 Compare August 3, 2021 21:16
@belav belav changed the title Working on workflow to validate check works Require directoryOrFile argument when not piping stdin Aug 3, 2021
@belav belav force-pushed the fix-check branch 2 times, most recently from 57e2aa1 to c3ceb1e Compare August 3, 2021 21:24
Formatting a file that wasn't already formatted

Working on powershell script for testing cli in workflow
@belav belav marked this pull request as ready for review August 3, 2021 21:25
@belav
Copy link
Owner Author

belav commented Aug 7, 2021

I wanted to use TestCli for recreating/testing #392, so I included that in this PR.
closes #392

Copy link
Collaborator

@shocklateboy92 shocklateboy92 left a comment

Choose a reason for hiding this comment

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

Okay, actually looked through the PS code this time.
Looks pretty good - made some minor comments that might be worth doing.

Also, is there an easy way of importing other scripts in powershell? You have a lot of repeated variables (especially involving paths) in your scripts. If you can extract them to some common file and import them, it's probably worth doing.

@@ -34,7 +34,7 @@ if ($firstRun)
& git reset --hard
& git checkout -b $preBranch

dotnet $csharpierProject\Src\CSharpier\bin\Release\net5.0\dotnet-csharpier.dll
dotnet $csharpierProject\Src\CSharpier\bin\Release\net5.0\dotnet-csharpier.dll .
Copy link
Collaborator

Choose a reason for hiding this comment

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

probably worth making a $csharpier variable like you do in TestCli.ps.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Also, github won't let me make comments on lines that are not part of the diff, but it's probably worth putting Release into a variable and referencing it here and in the build command.

You'll probably never change build configuration, but I'm a stickler for not repeating code 😏

@@ -46,7 +46,7 @@ foreach($folder in Get-ChildItem $csharpierRepos) {

Push-Location $csharpierRepos

dotnet $csharpierProject\Src\CSharpier\bin\Release\net5.0\dotnet-csharpier.dll
dotnet $csharpierProject\Src\CSharpier\bin\Release\net5.0\dotnet-csharpier.dll .
Copy link
Collaborator

Choose a reason for hiding this comment

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

The 2 comments I made about CreateTestingPr.ps1 apply to this file too.

@belav belav merged commit 863b117 into master Aug 15, 2021
@belav belav deleted the fix-check branch August 15, 2021 16:27
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.

Require the directoryOrFile argument when not piping into to stdin
2 participants