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 return error support to MetricsQueryRequest.WithStartEnd() #8392

Merged
merged 1 commit into from
Jun 17, 2024

Conversation

pracucci
Copy link
Collaborator

What this PR does

In #8374 I've an use case where WithStartEnd() may return error. In this PR I propose to add error return support to WithStartEnd(). We already use the same pattern in WithQuery(), reason why I don't think this change is much contentious.

Which issue(s) this PR fixes or relates to

N/A

Checklist

  • Tests updated.
  • Documentation added.
  • CHANGELOG.md updated - the order of entries should be [CHANGE], [FEATURE], [ENHANCEMENT], [BUGFIX].
  • about-versioning.md updated with experimental features.

Signed-off-by: Marco Pracucci <marco@pracucci.com>
@pracucci pracucci marked this pull request as ready for review June 17, 2024 10:03
@pracucci pracucci requested a review from a team as a code owner June 17, 2024 10:03

func mustSucceed[T any](value T, err error) T {
if err != nil {
panic(err)
Copy link
Contributor

@krajorama krajorama Jun 17, 2024

Choose a reason for hiding this comment

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

nit: To me panic in test implies that it's catching a programming error that should never happen in real life. I'd use require.NoError() or assert.NoError and pass t.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree with you. It's a bit annoying having to pass t because can't be a wrapper anymore. The closest thing would be:

func mustNoError[T any](t *testing.T) func(value T, err error) T {
       return func(value T, err error) T {
               require.NoError(t, err)
               return value
       }
}

And then you can call it like this:

-                       requestB:      mustSucceed(rangeQuery.WithStartEnd(rangeQuery.GetStart()+5*time.Minute.Milliseconds(), rangeQuery.GetEnd()+5*time.Minute.Milliseconds())),
+                       requestB:      mustNoError[MetricsQueryRequest](t)(rangeQuery.WithStartEnd(rangeQuery.GetStart()+5*time.Minute.Milliseconds(), rangeQuery.GetEnd()+5*time.Minute.Milliseconds())),

I think this latter version makes code a bit more ugly. Given the panic affects only tests I will move on.

Copy link
Contributor

@krajorama krajorama left a comment

Choose a reason for hiding this comment

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

approved with nit

@pracucci pracucci merged commit 337483b into main Jun 17, 2024
29 checks passed
@pracucci pracucci deleted the add-error-to-withstartend branch June 17, 2024 10:28
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.

2 participants