-
Notifications
You must be signed in to change notification settings - Fork 180
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
[Core] New Filters and Logic #9548
Conversation
7bea492
to
196a12a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few comments after code review, testing now
// phan only understands method_exists on simple variables, not | ||
// Assigning to a variable is the a workaround | ||
// for false positive 'getCenterIDs doesn't exist errors suggested | ||
// in https://github.com/phan/phan/issues/2628 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just adding this for documentation, #7309 was involved in this too
Co-authored-by: Saagar Arya <51128536+skarya22@users.noreply.github.com>
Co-authored-by: Saagar Arya <51128536+skarya22@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Tested with a custom script and all the filters work well.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most of these doc comments could have just been "{@inheritdoc}", but the ones written are fine too.
Brief summary of changes
This PR creates new filters and modifies existing ones to add functionality and make them more scalable. Two new composite filters can also be used to join other filters in an AND or OR logic. These filters should allow us to move away from writing a new filter with "combined" functionality for each module (i.e.
UserProjectMatchOrHasAnyPermissions
) instead you can use independent filters and join them in the order you need them to be.Changes in PR #9549 demo the use of these filters and how to combine them
PS these changes should not break existing code
Testing instructions (if applicable)
Link(s) to related issue(s)