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

Conditionally set url label for 404 responses #111

Merged
merged 9 commits into from
Mar 21, 2024

Conversation

therealak12
Copy link
Contributor

The MR addresses #86. I've explained about it in the original issue. TLDR; It adds a new field to the MiddlewareConfig to control the URL label when the response status code is 404.

@aldas
Copy link
Contributor

aldas commented Feb 29, 2024

Please add test(s)

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

Test(s) are missing

@therealak12
Copy link
Contributor Author

therealak12 commented Feb 29, 2024

I've added two tests covering the new scenario. Thanks for reminding me.
@aldas

@therealak12
Copy link
Contributor Author

Dear @aldas,
If you have a moment, would you be willing to kindly review the changes?

@aldas
Copy link
Contributor

aldas commented Mar 17, 2024

I think instead of SetPathFor404 *bool would DotNoUseURLFor404 bool would be better. Using pointers as configuration values is poor user experience.

@therealak12
Copy link
Contributor Author

I think instead of SetPathFor404 *bool would DotNoUseURLFor404 bool would be better. Using pointers as configuration values is a poor user experience.

Thanks, dear @aldas. I renamed the field.

About using pointers, I used a pointer to be able to distinguish between the default false values and a nil value. What do you prefer as an alternative for this?

I can think of setting a default true value here to be backward compatible but it's not possible for the NewMiddlewareWithConfig function as it should be able to distinguish between false and nil in the passed configuration.

@aldas
Copy link
Contributor

aldas commented Mar 18, 2024

I do not think we need 3 cases. We only need config flag to invert current behavior.

@therealak12
Copy link
Contributor Author

therealak12 commented Mar 18, 2024

I do not think we need 3 cases. We only need config flag to invert current behavior.

Sorry, I was missing the change in semantics after renaming the variable. It's fixed now.

echoprometheus/prometheus.go Outdated Show resolved Hide resolved
Copy link

codecov bot commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 64.08%. Comparing base (fdc9c9a) to head (48084db).

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #111   +/-   ##
=======================================
  Coverage   64.08%   64.08%           
=======================================
  Files           9        9           
  Lines         944      944           
=======================================
  Hits          605      605           
  Misses        300      300           
  Partials       39       39           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Contributor

@aldas aldas left a comment

Choose a reason for hiding this comment

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

LGTM

@aldas aldas merged commit 7605e2a into labstack:master Mar 21, 2024
12 checks passed
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