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

feat(dependency): replace go-spew package #1499

Merged
merged 10 commits into from Apr 22, 2024

Conversation

dongjiang1989
Copy link
Contributor

@dongjiang1989 dongjiang1989 commented Apr 12, 2024

use godebug package replace go-spew package

fix: #1490

@dongjiang1989
Copy link
Contributor Author

@kakkoyun PTAL

/cc @anurag-deshpande

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Thanks a lot for handling this.

I wonder how hard this is even without using a transitive dependency.

Could you maybe give it another shot with the standard library testing package?

@dongjiang1989
Copy link
Contributor Author

Thanks a lot for handling this.

I wonder how hard this is even without using a transitive dependency.

Could you maybe give it another shot with the standard library testing package?

Thanks for your review. Hmmm... Maybe not Diff string with the standard library testing package.

Like:

-(bytes.Buffer) # HELP some_other_metric A value that represents a counter.
-# TYPE some_other_metric counter
-some_other_metric{label1="value1"} 1
+(bytes.Buffer) # HELP some_total A value that represents a counter.
+# TYPE some_total counter
+some_total{label1="value1"} 1

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

It's great that the test are green now. But we are losing some information.

I'd suggest adding the following file with proper licence headers to the testutils package https://github.com/kylelemons/godebug/blob/master/diff/diff.go

https://pkg.go.dev/github.com/kylelemons/godebug@v1.1.0/diff

And updating the tests accordingly.

Or go back the previous version where you used go-cmp.

In any case, good job. You're close :)

prometheus/testutil/testutil_test.go Outdated Show resolved Hide resolved
prometheus/testutil/testutil_test.go Show resolved Hide resolved
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
This reverts commit bfbe25e.

Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
Copy link
Contributor Author

It's great that the test are green now. But we are losing some information.

I'd suggest adding the following file with proper licence headers to the testutils package https://github.com/kylelemons/godebug/blob/master/diff/diff.go

https://pkg.go.dev/github.com/kylelemons/godebug@v1.1.0/diff

And updating the tests accordingly.

Or go back the previous version where you used go-cmp.

In any case, good job. You're close :)

Thanks for your review. Please recheck :)

Copy link
Member

@kakkoyun kakkoyun left a comment

Choose a reason for hiding this comment

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

Great job 👏

Only one last thing.

I'd suggest adding the following file with proper licence headers to the testutils package https://github.com/kylelemons/godebug/blob/master/diff/diff.go

In my previous comment, I suggested copying over the diff.go file from this repo rather than adding as a dependency. Remember our goal is to reduce the dependencies. Since it's a single file and simple enough. We can just copy the files (with licence headers) into the testutils package.

Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
@dongjiang1989
Copy link
Contributor Author

Great job 👏

Only one last thing.

I'd suggest adding the following file with proper licence headers to the testutils package https://github.com/kylelemons/godebug/blob/master/diff/diff.go

In my previous comment, I suggested copying over the diff.go file from this repo rather than adding as a dependency. Remember our goal is to reduce the dependencies. Since it's a single file and simple enough. We can just copy the files (with licence headers) into the testutils package.

Got it. Thanks. Fixed

dongjiang1989 and others added 2 commits April 22, 2024 09:53
Signed-off-by: dongjiang1989 <dongjiang1989@126.com>
Signed-off-by: Kemal Akkoyun <kakkoyun@gmail.com>
@kakkoyun kakkoyun enabled auto-merge (squash) April 22, 2024 06:48
@kakkoyun kakkoyun merged commit 2f59eb2 into prometheus:main Apr 22, 2024
9 checks passed
@dongjiang1989 dongjiang1989 deleted the remove-go-spew branch April 22, 2024 09:18
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.

Remove go-spew dependency
2 participants