Open mmlb opened 2 years ago
path/filepath
-> path
same deal, path
-> encoding/json
does the right thing...
[17:08:52]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test
import (
"path"
"time"
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/yaml"
)
[17:08:54]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig 2022-04-25 17:08:54.102578903 -0400
+++ gofumpt_does_not_like_path_filepath 2022-04-25 17:08:54.102578903 -0400
@@ -3,7 +3,9 @@
import (
"path"
"time"
+
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
+
"sigs.k8s.io/yaml"
)
[17:09:03]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package tests_test
import (
"enconding/json"
"time"
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/yaml"
)
[17:09:06]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig 2022-04-25 17:09:06.095668682 -0400
+++ gofumpt_does_not_like_path_filepath 2022-04-25 17:09:06.095668682 -0400
@@ -3,6 +3,7 @@
import (
"enconding/json"
"time"
+
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/yaml"
Not sure I understand; gofumpt is meant to separate imports into groups, where the first group is the standard library. Isn't that what you are seeing?
@mvdan not quite. gofumpt is creating 3 import groups instead of just 2. I was expecting
import (
"path"
"time"
+
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"sigs.k8s.io/yaml"
)
And it does do that if I don't have path
in the import block. FYI I don't think the issue is with path itself for all cases, its just what seems to trigger this unexpected behavior in my imports. Here's another buggy example not tied to path:
Only 2 groups as expected:
[09:49:20]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package main
import (
"encoding/json"
"time"
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/types2"
"sigs.k8s.io/yaml"
)
[09:49:23]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig 2022-04-26 09:49:23.724699015 -0400
+++ gofumpt_does_not_like_path_filepath 2022-04-26 09:49:23.724699015 -0400
@@ -3,6 +3,7 @@
import (
"encoding/json"
"time"
+
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/types2"
But now I add package zz.zz/foo/bar
and now we get 3 groups, unexpectedly:
[09:49:40]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> cat gofumpt_does_not_like_path_filepath
package main
import (
"encoding/json"
"time"
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/types2"
"sigs.k8s.io/yaml"
"zz.zz/foo/bar"
)
[09:49:46]-[~/cloned/mvdan.cc/gofumpt]─[manny@zennix]> ./gofumpt -d gofumpt_does_not_like_path_filepath
diff -u gofumpt_does_not_like_path_filepath.orig gofumpt_does_not_like_path_filepath
--- gofumpt_does_not_like_path_filepath.orig 2022-04-26 09:49:46.932899018 -0400
+++ gofumpt_does_not_like_path_filepath 2022-04-26 09:49:46.932899018 -0400
@@ -3,9 +3,11 @@
import (
"encoding/json"
"time"
+
"github.com/tinkerbell/tink/pkg/apis/core/v1alpha1"
"k8s.io/apimachinery/pkg/types"
"k8s.io/apimachinery/pkg/types2"
"sigs.k8s.io/yaml"
+
"zz.zz/foo/bar"
)
Thank you - that is very weird, and I can reproduce.
It seems like what happens here is that our logic to reorder imports doesn't quite work in some edge cases - which is understandable given that we have to fix up the position offsets, and doing that while keeping empty lines and comments where they should be is... surprisingly hard. Properly fixing this might require a bit of a rethink.
yeah when I went trawling for an easy fix I noticed the same and thus brought the issue up instead of opening a PR :)
This is pretty strange but I noticed that gofumpt was creating unexpected groups in
import (...)
blocks. I've tracked it down to gofumpt doing the wrong thing whenpath/filepath
is imported (maybe others?). Here's what I'm seeing.I've seen the following on both v0.3.1 and latest master commit:
With
path/filepath
in the imports:and now without
path/filepath
:git bisect
says b7afc715b566b948526ad840fc20d53b642f6b6c is the first bad commit. This is the commit where stdlib is separated from other imports. Before this commit all the commits are kept grouped together, but at least handled right.Still with
path/filepath
in the imports: