Skip to content

Conversation

technicalpickles
Copy link
Contributor

@nateberkopec mentioned in passing how big the call method is. This is just my attempt to reduce it a little 😁

@technicalpickles technicalpickles changed the title Refactor initial skip logic into methods in MiniProfiler Refactor parameter query settings into its own class Oct 18, 2023
@technicalpickles
Copy link
Contributor Author

This PR kinda got away from me 😁 It ended up being more about abstracting what settings are being passed in via query parameters than the skip logic.

I made a QuerySettings which encapsulates the logic. I think this has nice parity with ClientSettings, because we have places that end up like query_settings.something || query_settings.something which feels nice.

As I was going through this, I saw that there are a bunch of places doing =~ to check matches, but not using the results. I changed those to match? to avoid allocating match data.

Copy link
Collaborator

@nateberkopec nateberkopec left a comment

Choose a reason for hiding this comment

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

This is a big improvement for readability. 👍

@technicalpickles
Copy link
Contributor Author

Have some merge conflicts to sort out in the wake of #593

@SamSaffron
Copy link
Member

I like this change a lot but yeah we need a rebase here.

@kbrock
Copy link
Contributor

kbrock commented Sep 26, 2025

Darn. This may be tricky to merge.

I had done something like this years back.
If it isn't merged right away, it easily becomes un-rebase-able.
(especially with 17 commits)

Thanks for this PR Josh. (and ✋ )

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.

4 participants