implement the --diff flag for the fmt command #150

danielj-jordan · 2018-10-20T19:07:32Z

I am a bit new to Go and eager for any comments. Thanks

nywilken

@danielj-jordan I took a quick pass-through and left a few comments pertaining to the runFmt function signature. I will continue looking through the PR and make suggestions where possible.

Thanks again for opening up the PR!

nywilken · 2018-10-21T09:48:24Z

cmd/fmt.go

 	if _, err := os.Stat(filepath.Join(outDir, "config")); os.IsNotExist(err) {
 		os.Mkdir(filepath.Join(outDir, "config"), os.ModePerm)
 	}
+	var diffFound = false


When assigning the zero value (default value) for a type it is preferred to just use var diffFound bool

nywilken · 2018-10-25T02:20:44Z

cmd/fmt.go

@@ -29,19 +32,24 @@ It also normalizes and alphabetizes the exercise topics in the config.json file.
 `,
 	Example: fmt.Sprintf("  %s fmt %s --verbose", binaryName, pathExample),
 	Run: func(cmd *cobra.Command, args []string) {
-		if err := runFmt(args[0], args[0], fmtVerbose); err != nil {
+
+		if diffFound, err := runFmt(args[0], args[0], fmtVerbose, fmtDiff); err != nil {


Seeing as fmtVerbose and fmtDiff are package level variables we should be able to access them from within runFmt without having to pass them in.

If you remove the parameters from here and within the runFmt function you would need to update the references of verbose and diffonly to fmtVerbose and fmtDiff

nywilken · 2018-10-25T02:31:59Z

cmd/fmt_example_test.go

@@ -24,7 +24,7 @@ func ExampleFormat() {
 	}
 	defer os.Remove(unformattedDir)

-	runFmt("../fixtures/format/unformatted/", unformattedDir, true)
+	runFmt("../fixtures/format/unformatted/", unformattedDir, true, false)


If you make the update so that runFmt uses fmtVerbose and fmtDiff directly you could remove the parameters and instead use the convention on lines 12-19 to set and reset the values for the pkg level variables fmtVerbose and fmtDiff.

oldVerbose = fmtVerbose fmtVerbose = true defer func() { fmtVerbose = oldVerbose }

nywilken · 2018-10-25T02:32:41Z

cmd/fmt_test.go

@@ -26,7 +26,7 @@ func TestFmtCommand(t *testing.T) {
 		t.Fatal(err)
 	}
 	defer os.Remove(formattedDir)
-	runFmt("../fixtures/format/formatted/", formattedDir, false)
+	runFmt("../fixtures/format/formatted/", formattedDir, false, false)


Same comment I left for fmt_example_test applies here.

Thanks @nywilken. I made the changes that you suggested. If you like the current code, I can squash and re-submit the PR if you like.

kytrinyx

This is looking great! I have a couple of tiny comments/suggestions, but I think this is basically good to go.

kytrinyx · 2018-11-02T14:19:38Z

cmd/fmt.go

 		}
 	}
 	if changes != "" {
-		ui.Print("changes made to:\n", changes)
+		if fmtDiff == false {


Here we basically have an "if not else".

I think it would make sense to swap it around so that it reads `if fmtDiff, otherwise..."

kytrinyx · 2018-11-02T14:20:10Z

cmd/fmt.go

@@ -105,7 +122,7 @@ func formatFile(cfg ConfigSerializer, inPath, outPath string) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	if diff != "" {
+	if diff != "" && fmtDiff == false {


It would be more idiomatic to make this !fmtDiff

kytrinyx · 2018-11-02T14:21:09Z

cmd/fmt.go

@@ -116,4 +133,5 @@ func formatFile(cfg ConfigSerializer, inPath, outPath string) (string, error) {
 func init() {
 	RootCmd.AddCommand(fmtCmd)
 	fmtCmd.Flags().BoolVarP(&fmtVerbose, "verbose", "v", false, "display the diff of the formatted changes.")
+	fmtCmd.Flags().BoolVarP(&fmtDiff, "diff", "d", false, "display the diff of the proposed changes, but do not make them.")


How would you feel about dry-run as the flag name instead of diff?
That's a very common flag name for something that would try something out, report what would happen, but then not actually make the changes.

@nywilken I'd love to hear your thoughts on this one as well.

I think it is better named dry-run also.

How would you feel about dry-run as the flag name instead of diff?

👍 I like that option over diff. I would even go so far as saying that we copy the option and messaging from the grep command

-n, --dry-run perform a trial run with no changes made

Actually, looking at the original issue we want a non-zero code if there are formatting issues so a dry-run might not be the option we want. In fact, I wouldn't expect dry-run to exit without error if the command successfully prints out the changes that would be written to the source file.

What if we keep the dry-run option, but also add a -t, --test flag that returns a non-zero if there are formatting issue?

-t, —test performs a trial and exits if formatting is needed; meant to be used with ci

What if we keep the dry-run option, but also add a -t, --test flag that returns a non-zero if there are formatting issue.

I pulled down the branch and see that dry-run does not actually print the changes unless the verbose flag is passed as well so I would like to change my previous suggestion slightly. Which of course is open for discussion.

Rename the dry-run option to test or check using the messaging above, enable verbosity any time the check flag is passed so that if the check fails users will see the diff. Thoughts?

@nywilken these are good observations. I think that it's important that we can exit with an error code in order to be able to use this in CI. In that sense, I think that test communicates this better than dry-run.

For the doctor command, I think we always need to print in the test version, whether or not verbose is passed. I think it's fine to not print verbose output if just running doctor to fix something.

kytrinyx · 2018-11-02T14:22:05Z

cmd/fmt_example_test.go

+	fmtVerbose = true
+	fmtDiff =  false
+
+	runFmt("../fixtures/format/unformatted/", unformattedDir)


I really like how much more readable this became now that the booleans are being assigned to the package variables!

danielj-jordan · 2018-11-03T01:30:43Z

@nywilken @kytrinyx Thanks for the feedback. I have incorporated all your recommendations and squashed the commit.

nywilken

@danielj-jordan @kytrinyx this is coming along nicely. I left a couple of suggestions along with my thoughts on the flag name. FWIW the code suggestions within this review should not have to change if we decide to rename the flag - only the references to the flag variable.

nywilken · 2018-11-04T10:52:31Z

cmd/fmt.go

 				ui.Print(fmt.Sprintf("%s\n\n%s", f.inPath, diff))
 			}
 			changes += fmt.Sprintf("%s\n", f.inPath)
+			diffFound = true
+
 		}
 	}
 	if changes != "" {


The declaration of diffFound can actually be moved right above this line with a slight change

Suggested change

if changes != "" {

diffFound := changes != ""

if diffFound {

...

nywilken · 2018-11-04T10:53:47Z

cmd/fmt.go

 	if _, err := os.Stat(filepath.Join(outDir, "config")); os.IsNotExist(err) {
 		os.Mkdir(filepath.Join(outDir, "config"), os.ModePerm)
 	}
+	var diffFound bool


This declaration can be made closer to where it is being used.

This should still be moved closer to where it's used 👍

nywilken · 2018-11-04T10:54:32Z

cmd/fmt.go

 				ui.Print(fmt.Sprintf("%s\n\n%s", f.inPath, diff))
 			}
 			changes += fmt.Sprintf("%s\n", f.inPath)
+			diffFound = true


If you apply the suggestions made below you can drop this line.

nywilken · 2018-11-04T10:57:34Z

cmd/fmt.go

 		}
 	}
 	if changes != "" {
-		ui.Print("changes made to:\n", changes)
+		if fmtDryRun {


Seeing as the flag description specifies that no changes will be made I would drop this if/else and instead only print when changes were made.

if diffFound && !fmtDryRun { ui.Print("changes made to:\n", changes) }

nywilken · 2018-11-04T10:59:35Z

cmd/fmt.go

 	}
 	if err := errs.ErrorOrNil(); err != nil {
-		return err
+		return diffFound, err


If you apply the suggestion on line 92 you can just return false with the error

nywilken · 2018-11-04T11:02:13Z

cmd/fmt.go

@@ -29,19 +35,24 @@ It also normalizes and alphabetizes the exercise topics in the config.json file.
 `,
 	Example: fmt.Sprintf("  %s fmt %s --verbose", binaryName, pathExample),
 	Run: func(cmd *cobra.Command, args []string) {
-		if err := runFmt(args[0], args[0], fmtVerbose); err != nil {
+


What do you think enabling verbosity anytime the format check is run? Something like the following right before we call runFmt(...)

if fmtDryRun { fmtVerbose = true }

Yes. It makes sense to do this

danielj-jordan · 2018-11-05T20:46:47Z

I want to confirm that the flag should be named —test not —dry-run. Please reply and confirm. Thank you

kytrinyx · 2018-11-05T20:52:21Z

@danielj-jordan thank you for confirming. Yes! We're going with --test rather than --dry-run.

Also, the command should

set verbose to true even if verbose wasn't passed, and
exit with a non-zero code if something needs changing

danielj-jordan · 2018-11-11T22:27:58Z

@kytrinyx @nywilken I am not sure if you got notified, but the —test flag is implemented. It always runs with verbose.

kytrinyx · 2018-11-12T05:43:09Z

@danielj-jordan Ah, nope! I didn't see the notification. Thanks so much, I'll take a look at this in the morning.

kytrinyx

There's one more minor tweak to do (the diffFound thing that @nywilken mentioned previously. After that I think this is good to go!

kytrinyx · 2018-11-13T14:06:05Z

cmd/fmt.go

 	if _, err := os.Stat(filepath.Join(outDir, "config")); os.IsNotExist(err) {
 		os.Mkdir(filepath.Join(outDir, "config"), os.ModePerm)
 	}
+	var diffFound bool


This should still be moved closer to where it's used 👍

danielj-jordan · 2018-11-13T15:00:48Z

@kytrinyx I moved the declaration closer. Thanks

kytrinyx

This is great, thanks so much @danielj-jordan!

Suggestions have been applied.

nywilken previously requested changes Oct 25, 2018

View reviewed changes

kytrinyx suggested changes Nov 2, 2018

View reviewed changes

danielj-jordan force-pushed the implement-fmt-diff-flag branch from 14b5327 to b702936 Compare November 3, 2018 01:16

nywilken reviewed Nov 4, 2018

View reviewed changes

danielj-jordan force-pushed the implement-fmt-diff-flag branch from b702936 to b4cb42b Compare November 10, 2018 00:52

kytrinyx suggested changes Nov 13, 2018

View reviewed changes

implement --test flag

771f30d

danielj-jordan force-pushed the implement-fmt-diff-flag branch from b4cb42b to 771f30d Compare November 13, 2018 14:39

Move variable declaration closer to where it is used

2456d3a

kytrinyx approved these changes Nov 15, 2018

View reviewed changes

kytrinyx merged commit 7c547b5 into exercism:master Nov 15, 2018

ekingery mentioned this pull request Oct 16, 2020

Add a --diff flag for fmt that will not actually rewrite the files #148

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

implement the --diff flag for the fmt command #150

implement the --diff flag for the fmt command #150

danielj-jordan commented Oct 20, 2018

nywilken left a comment

nywilken Oct 21, 2018

nywilken Oct 25, 2018 •

edited

Loading

nywilken Oct 25, 2018

nywilken Oct 25, 2018

danielj-jordan Oct 30, 2018

kytrinyx left a comment

kytrinyx Nov 2, 2018

kytrinyx Nov 2, 2018

kytrinyx Nov 2, 2018

danielj-jordan Nov 3, 2018

nywilken Nov 4, 2018

nywilken Nov 4, 2018 •

edited

Loading

nywilken Nov 4, 2018 •

edited

Loading

kytrinyx Nov 5, 2018

kytrinyx Nov 2, 2018

danielj-jordan commented Nov 3, 2018

nywilken left a comment

nywilken Nov 4, 2018

nywilken Nov 4, 2018

kytrinyx Nov 13, 2018

nywilken Nov 4, 2018

nywilken Nov 4, 2018

nywilken Nov 4, 2018

nywilken Nov 4, 2018

danielj-jordan Nov 5, 2018

danielj-jordan commented Nov 5, 2018

kytrinyx commented Nov 5, 2018

danielj-jordan commented Nov 11, 2018

kytrinyx commented Nov 12, 2018

kytrinyx left a comment

kytrinyx Nov 13, 2018

danielj-jordan commented Nov 13, 2018

kytrinyx left a comment

-	if changes != "" {
+diffFound := changes != ""
+if diffFound {
+...

implement the --diff flag for the fmt command #150

implement the --diff flag for the fmt command #150

Conversation

danielj-jordan commented Oct 20, 2018

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken Oct 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nywilken Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

nywilken Nov 4, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielj-jordan commented Nov 3, 2018

nywilken left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielj-jordan commented Nov 5, 2018

kytrinyx commented Nov 5, 2018

danielj-jordan commented Nov 11, 2018

kytrinyx commented Nov 12, 2018

kytrinyx left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danielj-jordan commented Nov 13, 2018

kytrinyx left a comment

Choose a reason for hiding this comment

nywilken Oct 25, 2018 •

edited

Loading

nywilken Nov 4, 2018 •

edited

Loading

nywilken Nov 4, 2018 •

edited

Loading