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

Add --replace argument #33

Merged
merged 3 commits into from
Nov 17, 2021
Merged

Conversation

chaudum
Copy link
Contributor

@chaudum chaudum commented Nov 12, 2021

--replace can be used multiple times to add replacements by specifying
the Go module name and the replacement module, similar to go mod edit -replace=. Version of the replacement can be specified with the
@version suffix in the replacement path.

Fixes #32

Signed-off-by: Christian Haudum christian.haudum@gmail.com

`--replace` can be used multiple times to add replacements by specifying
the Go module name and the replacement module, similar to `go mod edit
-replace=`. Version of the replacement can be specified with the
`@version` suffix in the replacement path.

Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum marked this pull request as draft November 15, 2021 13:38
Signed-off-by: Christian Haudum <christian.haudum@gmail.com>
@chaudum chaudum marked this pull request as ready for review November 15, 2021 14:43
Copy link
Contributor

@imiric imiric left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! It LGTM, just have some style suggestions.

I'd like for @mstoykov to take a look before merging, just in case we're missing something or if there's a simpler way of doing this.

cmd/xk6/main.go Outdated Show resolved Hide resolved
cmd/xk6/main.go Outdated Show resolved Hide resolved
Comment on lines +138 to +143
if got == "." {
t.Errorf("did not expand path")
}
if err != nil {
t.Errorf("failed to expand path")
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, we're not using stretchr/testify in the project yet 😞 Otherwise this would be simpler using require.NoError() and assert.NotEqual().

It's fine leaving it as is. We can change this everywhere once we add testify as a dependency.

@imiric imiric requested a review from mstoykov November 15, 2021 16:38
Copy link
Contributor

@mstoykov mstoykov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks for the PR 🙇

I kind of feel it will be nice if when we clean inputs (for example removing /) we actually tell users that this was done, just in case our assumptions about it not being valid come out to be false. But I also think in this case it's probably fine

@imiric imiric merged commit 153b991 into grafana:master Nov 17, 2021
@chaudum
Copy link
Contributor Author

chaudum commented Nov 18, 2021

Thanks @imiric @mstoykov !

@chaudum chaudum deleted the chaudum/add-replace-arg branch November 18, 2021 10:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add possibility to add replace to the generated go.mod
3 participants