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

import/metavar: Recognize empty imports #29

Merged
merged 1 commit into from
Jun 25, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.0.0/),
and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html).

## Unreleased
- No changes yet.
### Fixed
- ([#2]): Patches with named imports now support matching and manipulating any
import.

[#2]: https://github.com/uber-go/gopatch/issues/2

## 0.0.2 - 2020-11-04
### Fixed
Expand Down
53 changes: 6 additions & 47 deletions docs/PatchesInDepth.md
Original file line number Diff line number Diff line change
Expand Up @@ -413,13 +413,6 @@ can now update consumers of `foo.FooClient`.
[patch above to avoid stuttering]: #avoid-stutter-patch

```diff
@@
@@
import "example.com/foo"

-foo.FooClient
+foo.Client

@@
var foo identifier
@@
Expand All @@ -429,14 +422,6 @@ var foo identifier
+foo.Client
```

The first diff in this patch affects files that use unnamed imports, and the
second affects those that use named imports---regardless of name.

> *Note*: In a future version of gopatch, we'll need only the second patch to
> make this transformation. See also, [#2].

[#2]: https://github.com/uber-go/gopatch/issues/2

#### Changing imports

In addition to matching on imports, you can also change imports with gopatch.
Expand Down Expand Up @@ -519,42 +504,16 @@ The above will match, both, named and unnamed imports of
|---------------------------------------|-------------------------------------|
| `import foo "example.com/foo-go.git"` | `import foo "example.com/foo.git"` |
| `import bar "example.com/foo-go.git"` | `import bar "example.com/foo.git"` |
| `import "example.com/foo-go.git"` | `import foo "example.com/foo.git"`* |

> *This case is a known bug. See [#2] for more information.
>
> You can work around this by first explicitly matching and replacing the
> cases with unnamed imports first. For example, turn the patch above into two
> diffs, one addressing the unnamed imports, and one addressing the named.
>
> ```diff
> @@
> var x identifier
> @@
> -import "example.com/foo-go.git"
> +import "example.com/foo.git"
>
> foo.x
>
> @@
> var foo, x identifier
> @@
> -import foo "example.com/foo-go.git"
> +import foo "example.com/foo.git"
>
> foo.x
> ```
| `import "example.com/foo-go.git"` | `import "example.com/foo.git"` |

#### Best practices for imports

Given the known limitations and issues with imports highlighted above, the
best practices for matching and manipulating imports are:
Given the limitations of AST analysis, gopatch cannot always accurately guess a
package name for an import.

- Handle unnamed imports first. This will make sure that previously named
imports do not unintentionally become named.
- When matching any import, use a metavariable name that matches the name of
the imported package **exactly**. This name is used by gopatch to guess the
name of the package.
As a best practice, when manipulating imports, use a metavariable name that
matches the name of the imported package **exactly**. gopatch will use that as
the name of the package during source analysis.

```
# BAD | # GOOD
Expand Down
2 changes: 0 additions & 2 deletions e2e_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,6 @@ var testsToSkip = map[string]struct{}{
"add_error_param/a": {}, // https://github.com/uber-go/gopatch/issues/6
"func_within_a_func/two_dots": {},
"mismatched_dots/unnamed": {}, // https://github.com/uber-go/gopatch/issues/9
"noop_import/remove_all": {},
"noop_import/remove_some": {},
"switch_elision/body": {},
"select_elision/example": {},
"case_elision/a": {},
Expand Down
4 changes: 2 additions & 2 deletions examples/gomock-v1.5.0.patch
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,10 @@
# the test finishes running. We no longer need to call mockCtrl.Finish
# manually.
@@
var ctrl identifier
var ctrl, gomock identifier
var t expression
@@
import "github.com/golang/mock/gomock"
import gomock "github.com/golang/mock/gomock"

ctrl := gomock.NewController(t)
...
Expand Down
14 changes: 1 addition & 13 deletions internal/engine/file_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -553,7 +553,7 @@ func TestFile(t *testing.T) {
},
},
{
desc: "remove import",
desc: "remove unnamed import",
// -import "foo"
// bar
minus: &pgo.File{
Expand All @@ -580,18 +580,6 @@ func TestFile(t *testing.T) {
"func bar() { bar() }",
),
},
{
desc: "named import",
giveSrc: text.Unlines(
"package x",
`import baz "foo"`,
"func bar() { bar() }",
),
wantSrc: text.Unlines(
"package x",
"func bar() { bar() }",
),
},
},
},
{
Expand Down
Loading