remember original replace directive #24
Conversation
|
This should fix (at least partially) issue #8. |
| if err := f.AddReplace(mod, "", dir, ""); err != nil { | ||
| return errors.Wrap(err) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
If errors.Wrap knew to keep nil errors as nil, you'd be able to just do a single-line return here I think.
Though it would be a bit of an ugly line.
There was a problem hiding this comment.
drive-by comment, sorry. The more popular pkg/errors handles this case :)
https://github.com/pkg/errors/blob/c059e472/errors.go#L180-L183
There was a problem hiding this comment.
errors.Wrap does know to keep nil errors as nil, but it made for a bit of an ugly line so I avoided it here :)
cmdundo.go
Outdated
| // It's not a "was" comment. Just remove it (after this loop so we don't | ||
| // interfere with the current range statement). | ||
| drop[r.Old.Path] = true | ||
| delete(modMap, r.Old.Path) |
There was a problem hiding this comment.
you can move these two deletes out of the if/else
| } | ||
| for _, m := range modules { | ||
| fmt.Printf("dropped %s\n", m) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func splitWasComment(s string) *modfile.Replace { |
There was a problem hiding this comment.
why not implement this with regexp and capture groups? This kind of string handling code always looks complicated to me.
There was a problem hiding this comment.
yeah, not a bad idea. done.
| return tokens | ||
| } | ||
|
|
||
| func splitPathVersion(s string) (module.Version, bool) { |
There was a problem hiding this comment.
nit - why not return just module.Version, then check if it's non-nil? Some types, like go/token.Position, have an IsValid() bool method to make the code easier to write.
There was a problem hiding this comment.
not sure. I think the bool return is quite clear, and the code not much longer with it. i'll leave it for now.
| continue | ||
| } | ||
| // These checks shouldn't fail when checkCanReplace has been | ||
| // called previously, but check anyway just to be sure. |
There was a problem hiding this comment.
then maybe make them panics? otherwise you may have a bug in checkCanReplace, and you might never notice.
Or perhaps make these error messages more scary, like "this shouldn't happen, file a bug!". Though I tend to just make these be panics.
When we run `gohack get` on a go.mod file that contains a replace directive, we now remember the old information inside a comment, and restore it when running `gohack undo`. To do this, we need to be able to parse go.mod comments, so we now use the modfile package rather than `go mod edit`. We also use testscript to add some tests. More tests still needed :)
rogpeppe
force-pushed
the
010-remember-replace
branch
from
50ce597
to
6b14ebc
Compare
When we run
gohack geton a go.mod file that contains a replacedirective, we now remember the old information inside a comment,
and restore it when running
gohack undo.To do this, we need to be able to parse go.mod comments, so we
now use the modfile package rather than
go mod edit.We also use testscript to add some tests. More tests still needed :)