Check code after format #655

lpedrosa · 2020-01-28T20:26:16Z

Summary

Started adapting the CLI code so it uses the FakeHelpers.formatFileAsync as mentioned on #642

I have tried to respect the previous functionality, e.g.

When the file is unchanged it is still written to the provided TextWriter

A good side-effect of this changes is that we can now identify is a file is unchanged and opt not to do anything.

I haven't yet included the --check flag, please see the Notes section below

Let me know what you think @nojaf.

Notes

I also took some time to look through the project and gather some knowledge about the internals. Given that, I wrote down a couple of suggestions that require a discussion before I start slicing through the code:

The formatFileAsync returns a DU which may create or not temp files in the background. I was wondering if it could just return the modified content string instead. Callers can then decide what to do with it, rather than having to read the modified contents from the temp file.

type FormatResult =
    | Formatted of file: string * content: string // as opposed to the temp file
    | Unchanged of file: string
    | Error of file:string * Exception

I also have a couple of questions:

Have you considered testing the CLI? For example, I have noticed that the --stdin functionality doesn't work with this current version (I'll submit another PR to fix this).
Is the CLI output stable, or can it be changed? For example:
- When the CLI fails to format a file, it prints a massive stack trace. It could be more user friendly instead e.g. error: Failed to format file <x>: <error message>.
- Unchanged files should probably still be printed to stdout

src/Fantomas/FakeHelpers.fs

nojaf · 2020-01-29T07:33:27Z

Hey @lpedrosa , looking good so far!

I'm ok with changing FormatResult, however, the behaviour of the FAKE helper should stay the same. formatCode should not break.

Maybe mark formatFilesAsync,formatFileAsync and others internal so the public api of FakeHelpers is a bit more clear. You might need an [<assembly:InternalsVisibleToAttribute("Fantomas.Tests")>] for Fantomas.CoreGlobalTool as well.

Have you considered testing the CLI? For example, I have noticed that the --stdin functionality doesn't work with this current version (I'll submit another PR to fix this).

No, I've never really considered this. But if there is an easy way (that can run in CI), by all means, go for it.

Is the CLI output stable, or can it be changed? For example:
When the CLI fails to format a file, it prints a massive stack trace. It could be more user friendly instead e.g. error: Failed to format file : .
Unchanged files should probably still be printed to stdout

There is room for improvement there, I'm ok with changes to that area.

Unchanged files should probably still be printed to stdout

Yes, indeed.

lpedrosa · 2020-01-29T13:32:45Z

Thanks for the review @nojaf. I will focus on wrapping up this PR, by eliminating the need for the temp files and adding the check functionality along with some tests for that.

With regards to changing the tool output, do you want this as a separate PR, since it will affect the release version number?

The CLI tests are something I would like to look into, but probably not in this PR.

I've submitted another PR (#656) that fixes the --stdin functionality.

nojaf · 2020-01-29T14:12:13Z

With regards to changing the tool output, do you want this as a separate PR, since it will affect the release version number?

It is ok to include in this PR.
To me, this is more of improvement rather than a breaking change. The next version would be 3.2 so that would cover this feature. Might be somewhat debatable but you shouldn't worry about that.

I've submitted another PR (#656) that fixes the --stdin functionality.

Thanks!

Lastly, could you also update the documentation of the CLI?

* Uses FakeHelper's format family functions which classify the format result i.e. changed/unchanged/error, in preparation for a --check command (fsprojects#642)

nojaf · 2020-02-04T06:50:58Z

Hello @lpedrosa , do you need any assistance to finish this PR?

lpedrosa · 2020-02-04T09:40:30Z

I should be able to wrap this up today, thanks!

* Supports checking both files and folders * Always reports changes to stdout

* previously it was blocking for every element of the sequence and that was delaying the output massivelly * now it should be pretty responsive

lpedrosa · 2020-02-04T18:15:25Z

I have included the --check functionality in the CLI and have also updated the docs.

The code can be cleaned up a bit e.g. move the check functionality to a separate module, test it, etc.

I can do that afterwards, considering that you might want to release 3.2.x and I don't want to hold the release for that.

With regards to testing CLI modules or code, would you prefer another test project, or should I just reference the CLI project in the current test project.

src/Fantomas.CoreGlobalTool/Program.fs

nojaf · 2020-02-04T18:37:38Z

src/Fantomas.CoreGlobalTool/Program.fs

+                        |> Seq.exists isError
+
+                    if hasErrors then
+                        return 99


Any particular reason you went with 99? Just wondering though.

Ahah. Not really. We could go with your lucky number. For example black (the python formatter) chose 123.

I was secretly hoping it was a Jay-Z reference 😋

@jindraivanek do you have a lucky number? I'm ok with 99 or anything else that makes sense.

nojaf · 2020-02-04T18:45:30Z

I released 3.2 yesterday actually.
The proposal is to release a first 3.3 beta once this PR is merged. That way you don't really have to wait that long to use --check. And we can gather some feedback before marking the API as stable.

If you are up for it I would prefer to clean up and write tests in this PR.

With regards to testing CLI modules or code, would you prefer another test project, or should I just reference the CLI project in the current test project.

I would go with a separate project. Keep using NUnit is that is possible. That makes the most sense to me.

Very happy with the progress you've made! Big up!

lpedrosa · 2020-02-04T18:56:51Z

Thanks for the positive feedback @nojaf. I'm really enjoying helping out and getting my hands dirty with F#.

I will clean this up and add some tests then.

* Added CoreGlobalTool.Test project * Test specific CoreGlobalTool functionality * Separated --check command logic into a separate module

lpedrosa · 2020-02-07T18:32:30Z

I have added a test project for the CLI functionality. I have also refactored main logic of the check command to a separate module and tested that.

I'm just missing the check that @nojaf highlighted above where "if there are no changes and the output file is the same, it shouldn't write anything at all"

lpedrosa · 2020-02-07T18:47:31Z

I'm just missing the check that @nojaf highlighted above where "if there are no changes and the output file is the same, it shouldn't write anything at all"

Actually, I suggest we merge this PR in. I have an idea how to tackle the above and it will require refactoring some of the processX functions and friends. That will probably be very involved, and another PR will be easier to review.

nojaf

Overall very nice work @lpedrosa !
If some minor things could be moved around we can merge this in.

fantomas.sln

src/Fantomas.CoreGlobalTool.Tests/Program.fs

src/Fantomas.CoreGlobalTool/Check.fs

src/Fantomas.CoreGlobalTool.Tests/CheckTests.fs

nojaf · 2020-02-14T10:50:15Z

Hey @lpedrosa, I've found some time to tackle some of the remarks I had.
Could you take a look at these latest changes I've made?

For the most part, I moved some things around so that the same functionality can be easily exposed via FAKEHelpers module.
The main idea is that the caller of the code can decide what they do with the result.
In case the CLI is the caller, I've processed the output as you did.

I hope you don't mind me tinkering with your PR, let me know what you think.

lpedrosa · 2020-02-15T13:47:09Z

Hey @nojaf, I've been away this week. I'll have a look at these changes hopefully on Monday.

I hope you don't mind me tinkering with your PR, let me know what you think

Feel free to thinker away :)

src/Fantomas.CoreGlobalTool.Tests/TestHelpers.fs

src/Fantomas/FakeHelpers.fs

nojaf · 2020-02-21T13:44:29Z

Ok, I think we can merge this now.
Unless you think otherwise @lpedrosa?

lpedrosa · 2020-02-21T13:45:33Z

Looks good to me! 🚀

lpedrosa requested a review from nojaf January 28, 2020 20:26

nojaf reviewed Jan 29, 2020

View reviewed changes

src/Fantomas/FakeHelpers.fs Outdated Show resolved Hide resolved

lpedrosa added 6 commits January 29, 2020 19:21

adapt process to use formatFileAsync API

d6e8afd

CoreGlobalTool checks if formatted code is valid

6590d4f

* Uses FakeHelper's format family functions which classify the format result i.e. changed/unchanged/error, in preparation for a --check command (fsprojects#642)

fix bug where stdin would not accept cat files

d851650

update --stdin flag docs

ad8d434

address code review concerns

57013d2

remove need for temp files in FormatResult

4e4c513

lpedrosa force-pushed the check-code-after-format branch from 47e3163 to 4e4c513 Compare January 30, 2020 14:24

nojaf added 2 commits February 3, 2020 19:21

Updated month in release notes

4b51b00

Merge branch 'master' into check-code-after-format

1b68cf7

nojaf and others added 4 commits February 4, 2020 11:28

Merge branch 'master' of https://github.com/nojaf/fantomas

8a7c550

Initial CLI check functionality draft

4fa6c28

* Supports checking both files and folders * Always reports changes to stdout

make check code faster

c602c74

* previously it was blocking for every element of the sequence and that was delaying the output massivelly * now it should be pretty responsive

update docs

106d111

nojaf reviewed Feb 4, 2020

View reviewed changes

src/Fantomas.CoreGlobalTool/Program.fs Show resolved Hide resolved

nojaf reviewed Feb 4, 2020

View reviewed changes

nojaf and others added 5 commits February 6, 2020 12:12

Merge branch 'master' into check-code-after-format

7df3d06

Merge remote-tracking branch 'upstream/master'

fa8a37c

add test CLI test project

4392aa3

Merge remote-tracking branch 'upstream/master'

b7788e9

Merge branch 'master' of https://github.com/nojaf/fantomas

d43194b

Refactor and test check functionality

d08dea7

* Added CoreGlobalTool.Test project * Test specific CoreGlobalTool functionality * Separated --check command logic into a separate module

nojaf requested changes Feb 8, 2020

View reviewed changes

nojaf added 6 commits February 10, 2020 10:01

Merge remote-tracking branch 'upstream/master'

d4e150c

Merge remote-tracking branch 'upstream/master'

c55b397

Removed src folder from solution

f6deb73

Removed nunitlite

cbd474e

Merged latest master

c81eb3a

Refactored checkCode to FAKEHelpers

d407053

Merge branch 'master' into check-code-after-format

8f2e66a

lpedrosa commented Feb 21, 2020

View reviewed changes

src/Fantomas.CoreGlobalTool.Tests/TestHelpers.fs Show resolved Hide resolved

src/Fantomas/FakeHelpers.fs Show resolved Hide resolved

Merge branch 'master' into check-code-after-format

521e0e7

nojaf self-requested a review February 21, 2020 13:43

nojaf approved these changes Feb 21, 2020

View reviewed changes

nojaf merged commit dfef3d5 into fsprojects:master Feb 21, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check code after format #655

Check code after format #655

lpedrosa commented Jan 28, 2020 •

edited

Loading

nojaf commented Jan 29, 2020

lpedrosa commented Jan 29, 2020

nojaf commented Jan 29, 2020

nojaf commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

nojaf Feb 4, 2020

lpedrosa Feb 4, 2020 •

edited

Loading

nojaf Feb 4, 2020 •

edited

Loading

nojaf commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

lpedrosa commented Feb 7, 2020

lpedrosa commented Feb 7, 2020

nojaf left a comment

nojaf commented Feb 14, 2020

lpedrosa commented Feb 15, 2020

nojaf commented Feb 21, 2020

lpedrosa commented Feb 21, 2020

Check code after format #655

Check code after format #655

Conversation

lpedrosa commented Jan 28, 2020 • edited Loading

Summary

Notes

nojaf commented Jan 29, 2020

lpedrosa commented Jan 29, 2020

nojaf commented Jan 29, 2020

nojaf commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

nojaf Feb 4, 2020

Choose a reason for hiding this comment

lpedrosa Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

nojaf Feb 4, 2020 • edited Loading

Choose a reason for hiding this comment

nojaf commented Feb 4, 2020

lpedrosa commented Feb 4, 2020

lpedrosa commented Feb 7, 2020

lpedrosa commented Feb 7, 2020

nojaf left a comment

Choose a reason for hiding this comment

nojaf commented Feb 14, 2020

lpedrosa commented Feb 15, 2020

nojaf commented Feb 21, 2020

lpedrosa commented Feb 21, 2020

lpedrosa commented Jan 28, 2020 •

edited

Loading

lpedrosa Feb 4, 2020 •

edited

Loading

nojaf Feb 4, 2020 •

edited

Loading