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

Implement Check_Collection to separate concerns for check filtering from Check_Repository #315

Merged
merged 9 commits into from
Nov 6, 2023

Conversation

felixarntz
Copy link
Member

This PR introduces a Check_Collection interface, primarily with the purpose of providing enhanced filtering of checks to run, while decoupling that from the Check_Repository responsibilities.

The Check_Repository was introduced as the central place to register checks, and while a limited flag based selection mechanism makes sense in the scope of that class, the parameter for filtering down to specific checks wasn't really well thought through, which becomes more obvious when considering #308:

  • We shouldn't just add more and more parameters to Check_Repository::get_checks() to allow further ways to filter.
  • At the same time, we should arbitrarily implement filtering partially in Check_Repository and partially elsewhere.

The Check_Collection approaches relies on sort of a "builder" pattern where Check_Repository::get_checks() returns a collection that allows further filtering and largely can be used like an array. A to_array() method and a to_map() method are present to return the under-the-hood array representation. Internally, the checks are stored in the collection with their slugs to provide the necessary filtering and lookup capabilities.

@felixarntz felixarntz added [Type] Enhancement A suggestion for improvement of an existing feature Infrastructure Issues for the overall plugin infrastructure Code Quality labels Oct 31, 2023
@felixarntz
Copy link
Member Author

felixarntz commented Oct 31, 2023

It's worth noting that this PR also fixes a bug: Before this, Check_Repository::get_checks() would return a map ($slug => $check) when no $check_slugs were passed, but an indexed array of $check objects when $check_slugs were passed. This was simply due to an oversight. It can be seen especially in the tests updated in this PR: The one test where a slug was passed to Check_Repository::get_checks() is the only scenario which now uses to_array(), and it is the only place where an indexed array was expected before in the test - which didn't make sense and was probably implemented like that simply to make the test pass.

Now, it returns the collection and then the caller can decide whether they need the map or array format, via to_map() / to_array().

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thank you, @felixarntz, for the PR. It looks quite solid from my perspective. I've left some feedback to seek clarification on certain points.

Copy link
Member

@mukeshpanchal27 mukeshpanchal27 left a comment

Choose a reason for hiding this comment

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

Thanks @felixarntz for the changes. LGTM from my end.

@swissspidy swissspidy merged commit 204dadd into trunk Nov 6, 2023
11 checks passed
@swissspidy swissspidy deleted the enhance/separate-check-repository-from-filtering branch November 6, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Code Quality Infrastructure Issues for the overall plugin infrastructure [Type] Enhancement A suggestion for improvement of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants