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

add missing raise for formatting functions #1376

Merged
merged 3 commits into from
Jan 22, 2021
Merged

add missing raise for formatting functions #1376

merged 3 commits into from
Jan 22, 2021

Conversation

btzo
Copy link
Contributor

@btzo btzo commented Jan 21, 2021

fix #1340

Running Fantomas.CoreGlobalTool to format a file without the --check argument, it does the treatment in the else listed below, calling the appropriate function given type of inputPath and outputPath.

if check then
inputPath |> runCheckCommand recurse |> exit
else
try
match inputPath, outputPath with
| InputPath.Unspecified, _ ->
eprintfn "Input path is missing..."
exit 1
| InputPath.File f, _ when (IgnoreFile.isIgnoredFile f) -> printfn "'%s' was ignored" f
| InputPath.Folder p1, OutputPath.Notknown -> processFolder p1 p1
| InputPath.File p1, OutputPath.Notknown -> processFile p1 p1
| InputPath.File p1, OutputPath.IO p2 -> processFile p1 p2
| InputPath.Folder p1, OutputPath.IO p2 -> processFolder p1 p2
| InputPath.StdIn s, OutputPath.IO p -> stringToFile s p FormatConfig.Default
| InputPath.StdIn s, OutputPath.Notknown
| InputPath.StdIn s, OutputPath.StdOut -> stringToStdOut s FormatConfig.Default
| InputPath.File p, OutputPath.StdOut -> fileToStdOut p
| InputPath.Folder p, OutputPath.StdOut -> allFiles recurse p |> Seq.iter fileToStdOut
with exn ->
printfn "%s" exn.Message
exit 1

The problem is that when an error occurred in one of these functions, it was treated locally but not propagated,
so the try/with condition that changes the error code to 1 is never achieved.

The ideal solution would be to show only the error messages in the last try\with and add tests for each function in the match expression, besides the simple test that I inserted.

What do you think @nojaf ?

@@ -28,11 +28,17 @@ let ``formatted files should report exit code 0`` () =
exitCode |> should equal 0

[<Test>]
let ``invalid files should report exit code 1`` () =
let ``invalid files should report exit code 1 with check`` () =
Copy link
Contributor

Choose a reason for hiding this comment

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

I would remove this with check part, it is implied by the file itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fine

use fileFixture = new TemporaryFileCodeSample(WithErrors)
let (exitCode, _) = checkCode fileFixture.Filename
exitCode |> should equal 1

[<Test>]
Copy link
Contributor

Choose a reason for hiding this comment

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

I would move this to a new file ExitCodeTests as this is not really related to --check.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think it's better this way.

In that case you think I should replicate the other tests from CheckTests to ExitCodeTests, replacing the function checkCode by the function formatCode ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm, I won't force you to replicate all tests. I feel like the one you have covered the need here.
The cli application isn't perfect so in this case, I would blink an eye for not having tests that cover all changes.
Maybe just clean up what you have and we can merge this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok

@nojaf
Copy link
Contributor

nojaf commented Jan 22, 2021

Hey Mike, that sounds about right.
Thanks for this effort!

@nojaf nojaf merged commit 232a644 into fsprojects:master Jan 22, 2021
@nojaf
Copy link
Contributor

nojaf commented Jan 22, 2021

Thanks again @btzo!

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.

Failing to format file should return an exit code different than 0
2 participants