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.

Sign up for GitHub

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

Jump to bottom

Refactor parameter query settings into its own class #592

Open
wants to merge 17 commits into
base: master
Choose a base branch
from
Open

Refactor parameter query settings into its own class #592

wants to merge 17 commits into from

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.

nateberkopec
nateberkopec approved these changes Nov 2, 2023
View reviewed changes
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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nateberkopec nateberkopec nateberkopec approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@technicalpickles @SamSaffron @nateberkopec