Add --no-code-fmt option #98

abhinav · 2022-10-11T20:06:43Z

Kunde21/markdownfmt attempts to reformat all snippets using the "go"
language type.

So it will rewrite anything like the following:

```go
package main

func foo() {
  // intentionally using two spaces
}
```

This change adds a --no-code-fmt command line option that opts out of this
behavior in markdownfmt by replacing the list of code formatters.

The option is named no-code-fmt instead of no-gofmt to account
for other code formatters that mdox may introduce in the future.

This change requires Kunde21/markdownfmt#38.
This change also depends on #96.

This temporarily pins to my forks of mdox and markdownfmt to include the following two changes: - bwplotka/mdox#98 - bwplotka/mdox/pull/98 These two combined add the ability for us to opt out of mdox reformatting all Go code blocks inside Markdown files. The .gitignore change is added because this version of mdox uses a local sqlite cache for speed.

This temporarily pins to my forks of mdox and markdownfmt to include the following two changes: - bwplotka/mdox#98 - Kunde21/markdownfmt#38 These two combined add the ability for us to opt out of mdox reformatting all Go code blocks inside Markdown files. The .gitignore change is added because this version of mdox uses a local sqlite cache for speed.

saswatamcode

Not sure I understand the motivations behind this change.

Why not reformat Go code with gofmt? It's enabled by default on every IDE and it is by design opinionated and non-configurable.

Based on PRs you linked, uber-go/fx#961, you mention that the gofmt formatting (via mdox) is inconsistent. Maybe addressing that issue, instead of disabling it altogether would be a better approach here? Wdyt? 🙂

abhinav · 2022-10-13T16:19:19Z

@saswatamcode our primary reasons for wanting this are:

Examples aren't always valid Go code.
gofmt will fail to reformat in these cases,
so some examples will be fully formatted and some won't.
Examples are sometimes intentionally incorrect.
In this case, reformatting would actually take away the point
that the example wanted to make.

In either case, it makes sense to have an escape hatch.

abhinav · 2022-10-13T16:22:24Z

Rebased on top of #96 and dropped the temporary pin-to-fork.

abhinav · 2022-10-13T16:28:46Z

Just to elaborate on #98 (comment) with some example scenarios:

Incremental step-by-step instructions don't always have valid code. For example, imagine something like,
1. Start a new main function.
```
func main() {
```
2. Initialize a new application.
```
func main() {
    app := fx.New(
    )
```
3. ...and so on
In these cases, we won't have valid Go code until we get to the end,
and gofmt will not reformat anything until that point.
In a style-guide context, you want to demonstrate good vs bad examples
where having no reformatting is desirable.

(FWIW, all our code samples are coming from external files that are properly gofmt-ed.)

This temporarily pins to my fork of mdox to include bwplotka/mdox#98. Separately, this upgrades markdownfmt to pick up Kunde21/markdownfmt#38. These two combined add the ability for us to opt out of mdox reformatting all Go code blocks inside Markdown files. The .gitignore change is added because this version of mdox uses a local sqlite cache for speed.

saswatamcode

In a style-guide context, you want to demonstrate good vs bad examples
where having no reformatting is desirable.

This use case makes sense to some extent, but would a style guide redefine what is already covered by gofmt?

Anyways, it can still be a useful debugging option, so I do not want to block it! Let's wait for @bwplotka's opinions too. 🙂

Also some small suggestions!

saswatamcode · 2022-10-14T08:00:13Z

go.mod

@@ -1,10 +1,9 @@
 module github.com/bwplotka/mdox

-go 1.15
+go 1.19


I think you raised PR for this separately #96?

Yeah, this PR is on top of that one. If #96 gets merged, this won't show those commits.
(Without stacking them, there are conflicts because of the dependency updates.)

saswatamcode · 2022-10-14T08:02:04Z

pkg/mdformatter/mdformatter_test.go

@@ -147,3 +147,18 @@ func TestCheck_SoftWraps(t *testing.T) {
 	testutil.Ok(t, err)
 	testutil.Equals(t, string(exp), diff.String())
 }
+
+func TestFormat_NoGofmt(t *testing.T) {


Thanks for the separate test case. I would also check what happens to this go code sample when --no-gofmt is disabled, just to ensure we aren't changing default behavior

bwplotka · 2022-10-14T08:40:50Z

👍🏽 I think it definitely makes sense to be able to disable gofmt - solid use cases. Perhaps doing this with extra directive on code snippet would do? Essentially opt-out for formatting per snippet?

abhinav · 2022-10-15T02:49:53Z

👍🏽 I think it definitely makes sense to be able to disable gofmt - solid use cases. Perhaps doing this with extra directive on code snippet would do? Essentially opt-out for formatting per snippet?

Thanks @bwplotka, I can add that to this PR too.
Are you still okay with the --no-gofmt top-level flag?
For our documentation, we'll otherwise end up adding the per-snippet opt-out on all snippets.

abhinav · 2022-10-15T02:54:07Z

I can call this --no-fmt and the per-snippet-opt-out mdox-fmt="false".
This way, if mdox adds reformatting for other languages, the same opt-out works.

saswatamcode · 2022-10-16T16:42:44Z

I think flag + directive makes sense here! The no-fmt flag would basically switch default to mdox-fmt=true.

abhinav · 2022-10-16T23:54:06Z

On second thought, per-snippet opt-out is possible,
but it either requires new APIs in markdownfmt to have a hook before it runs a reformat,
or we change it so that mdox always opts out of reformatting from markdownfmt,
and does its own reformatting if necessary.

For now, I've updated this PR with additional tests as suggested,
and renamed the flag to --no-code-fmt to opt out of all code reformatting.
This will account for new code reformatting logic being added in the future.

I can explore per-snippet opt-out separately from the global opt-out added by this flag.

abhinav · 2022-10-25T18:11:52Z

@bwplotka @saswatamcode
Any appetite for file-level opt-out with --no-code-fmt?
I don't know if I have time to implement per-snippet opt-out in markdownfmt at this time.

saswatamcode · 2022-10-26T15:50:03Z

Ack! WIll try to TAL this week, and see what would be nice to implement here! Thanks for the awesome work! 🙂

Update to latest markdownfmt master to pull in Kunde21/markdownfmt#38.

Kunde21/markdownfmt attempts to reformat all snippets using the "go" language type. So it will rewrite anything like the following: ```go package main func foo() { // intentionally using two spaces } ``` This change adds a --no-gofmt command line option that opts out of this behavior in markdownfmt by replacing the list of code formatters. This change requires Kunde21/markdownfmt#38 to build.

The scope of this flag is expanding to all code samples. If mdox supports more code reformatting in the future, this flag opts out of all of them.

Adds a test to verify the behavior of mdox with and without poorly formatted Go code. By default, it'll reformat. With the "--no-code-fmt" flag, it won't.

abhinav · 2022-10-26T15:53:51Z

Ack! WIll try to TAL this week, and see what would be nice to implement here! Thanks for the awesome work! 🙂

Thank you! I've rebased the PR now that #96 is in.

abhinav · 2022-12-12T20:30:42Z

Superseded by #99

abhinav mentioned this pull request Oct 11, 2022

[WIP] docs: Disable Go reformatting uber-go/fx#961

Closed

abhinav changed the title ~~Add --no-gofmt option~~ [don't land yet] Add --no-gofmt option Oct 11, 2022

saswatamcode reviewed Oct 13, 2022

View reviewed changes

abhinav force-pushed the nogofmt branch from b8b3b8e to 9825891 Compare October 13, 2022 16:21

abhinav changed the title ~~[don't land yet] Add --no-gofmt option~~ Add --no-gofmt option Oct 13, 2022

abhinav force-pushed the nogofmt branch from 9825891 to 7983f28 Compare October 13, 2022 16:30

saswatamcode reviewed Oct 14, 2022

View reviewed changes

abhinav changed the title ~~Add --no-gofmt option~~ Add --no-code-fmt option Oct 17, 2022

abhinav mentioned this pull request Oct 25, 2022

Use Go 1.19 #96

Merged

abhinav added 4 commits October 26, 2022 08:53

go.mod: Update markdownfmt

34f38bc

Update to latest markdownfmt master to pull in Kunde21/markdownfmt#38.

Rename --no-gofmt to --no-code-fmt

e89a72e

The scope of this flag is expanding to all code samples. If mdox supports more code reformatting in the future, this flag opts out of all of them.

test: badly formatted code with and without opt-out

52198e8

Adds a test to verify the behavior of mdox with and without poorly formatted Go code. By default, it'll reformat. With the "--no-code-fmt" flag, it won't.

abhinav force-pushed the nogofmt branch from 7449848 to 52198e8 Compare October 26, 2022 15:53

abhinav mentioned this pull request Dec 12, 2022

Upgrade markdownfmt, add opt-out for gofmt #99

Merged

abhinav closed this Dec 12, 2022

abhinav deleted the nogofmt branch December 12, 2022 20:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add --no-code-fmt option #98

Add --no-code-fmt option #98

abhinav commented Oct 11, 2022 •

edited

Loading

saswatamcode left a comment

abhinav commented Oct 13, 2022

abhinav commented Oct 13, 2022

abhinav commented Oct 13, 2022

saswatamcode left a comment

saswatamcode Oct 14, 2022

abhinav Oct 15, 2022

saswatamcode Oct 14, 2022

bwplotka commented Oct 14, 2022

abhinav commented Oct 15, 2022

abhinav commented Oct 15, 2022

saswatamcode commented Oct 16, 2022

abhinav commented Oct 16, 2022 •

edited

Loading

abhinav commented Oct 25, 2022

saswatamcode commented Oct 26, 2022

abhinav commented Oct 26, 2022

abhinav commented Dec 12, 2022

Add --no-code-fmt option #98

Add --no-code-fmt option #98

Conversation

abhinav commented Oct 11, 2022 • edited Loading

saswatamcode left a comment

Choose a reason for hiding this comment

abhinav commented Oct 13, 2022

abhinav commented Oct 13, 2022

abhinav commented Oct 13, 2022

saswatamcode left a comment

Choose a reason for hiding this comment

saswatamcode Oct 14, 2022

Choose a reason for hiding this comment

abhinav Oct 15, 2022

Choose a reason for hiding this comment

saswatamcode Oct 14, 2022

Choose a reason for hiding this comment

bwplotka commented Oct 14, 2022

abhinav commented Oct 15, 2022

abhinav commented Oct 15, 2022

saswatamcode commented Oct 16, 2022

abhinav commented Oct 16, 2022 • edited Loading

abhinav commented Oct 25, 2022

saswatamcode commented Oct 26, 2022

abhinav commented Oct 26, 2022

abhinav commented Dec 12, 2022

abhinav commented Oct 11, 2022 •

edited

Loading

abhinav commented Oct 16, 2022 •

edited

Loading