palkan / n_plus_one_control

RSpec and Minitest matchers to prevent N+1 queries problem
MIT License
553 stars 20 forks source link

Do not count a query matching .ignore pattern #47

Closed mrzasa closed 3 years ago

mrzasa commented 3 years ago

What is the purpose of this pull request?

There is a setting NPlusOneControl.ignore that is documented but not used.

What changes did you make? (overview)

I added a guard clause to Executor::Collector that skips queries matching the ignore pattern

Is there anything you'd like reviewers to focus on?

Two questions for potential future improvements (I may provide them as follow-up PRs)

  1. Could we use an array for ignore instead of a single regex? That'd be easier to write, would potentially run faster (regex alternative isn't the most performant thing) and would be aligned with DBQueryMatchers so it'd be easier to keep those two configs in sync (or to migrate).
  2. What do you think about removing the global state (direct calls to NPlusOneControl constant) from the Executor and inject the config in the constructor? It may make testing easier and save us from concurrency issues.

Checklist

Earendil95 commented 3 years ago

Hey @mrzasa 👋🏻

Thank you for your attention and the PR :)

Firstly, CI is red, please, take a look. Secondly,

I've added a Changelog entry Do we need that for a bugfix?

I think we do, just mention there you've fixed the issue.

Apart from that, looks good :)

palkan commented 3 years ago

NPlusOneControl.ignore that is documented but not used.

🙀

Could we use an array for ignore instead of a single regex?

Sounds good to me 👍

What do you think about removing the global state (direct calls to NPlusOneControl constant) from the Executor and inject the config in the constructor?

I would prefer to have both: global default and optional constructor parameter (similarly to default_matching).

Earendil95 commented 3 years ago

(I may provide them as follow-up PRs)

Just noticed that 🤦🏻‍♂️ Then, please, clean up outputs and I'll be ready to merge this

mrzasa commented 3 years ago

Done, please merge