-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Provide API to filter unwanted contributions #9317
Conversation
13c96d0
to
4ae3ca5
Compare
examples/api-samples/src/browser/contribution-filter/sample-filtered-command-contribution.ts
Show resolved
Hide resolved
examples/api-samples/src/browser/contribution-filter/sample-filtered-command-contribution.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
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.
Thanks for the contribution! Since this is going to be something core to Theia, I wanted to properly review it. Unfortunately I found enough things that bothered that I created a branch with most changes I'd be more comfortable with, see: eclipsesource/theia@filter-api...eclipse-theia:mp/filter-api-2
My issues mostly revolve around:
-
Class hierarchies too deep for not much value. Take the following:
@injectable() export class SampleFilteredCommandContributionFilter extends NameBasedContributionFilter { contributions = [CommandContribution]; doTest(toTest: string): boolean { return equals(toTest, false, 'SampleFilteredCommandContribution'); } }
It takes 3 abstract classes just not to write:
return equals(toTest.constructor.name, false, 'SampleFilteredCommandContribution');
It also seems to me that filters won't really be extended in such a way that we'll need crazy extensibility. Correct me if I am wrong.
-
Some of the logic can be rewritten more concisely.
-
Some styling issues. It most likely means that our linting rules are not good enough?
If you agree with the changes I proposed on the mp/filter-api-2
branch, feel free to squash them into your commit, populating the Co-Authored-By: Paul ...
line for IP stuff. The commit I made already contains fixes for most of what I reported in my review.
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/string-based-filter.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/string-based-filter.ts
Outdated
Show resolved
Hide resolved
Thanks @marechal-p for your in-depth review and the proposed changes.I squashed your changes and added the |
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.
This is a valuable contribution, but I think it would benefit from being done in a more functional style.
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Outdated
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter.ts
Outdated
Show resolved
Hide resolved
@paul-marechal I have squashed your proposal for a more functional filtering approach and rebased the PR onto the latest master. |
@tortmayr just so you know, Paul is OOO this week, so it could take a few business days for a response, from him. |
28c0b14
to
6b65888
Compare
Thanks for the re-review @paul-marechal. I removed the |
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.
I like the general shape of this PR a lot now. I have a couple of questions I feel need answers though, hence a "Request changes".
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Show resolved
Hide resolved
packages/core/src/common/contribution-filter/contribution-filter-registry.ts
Show resolved
Hide resolved
@tortmayr @paul-marechal so how are we going to proceed here? |
Waiting for @tortmayr to address the unaddressed comments? |
Adds the ContributionFilterRegistry which can be used to filter contributions of Theia extensions before they are bound. This mechanism can be used by application developers to specifically filter individual contributions from Theia extensions they have no control over, i.e. core or 3rd party extensions. Resolves eclipse-theia#9069 Contributed on behalf of STMicroelectronics Signed-off-by: Tobias Ortmayr <tortmayr@eclipsesource.com> Co-Authored-By: Paul Maréchal <paul.marechal@ericsson.com> squash me? Signed-off-by: Paul Maréchal <paul.marechal@ericsson.com>
@paul-marechal I have rebased the PR and addressed the open comments. |
@tsmaeder @paul-marechal : Are you good with this PR now? |
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.
Looks good to me now.
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 too, thanks!
What it does
Adds the
ContributionFilterRegistry
which can be used to filter contributions of Theia extensions before they are bound.This mechanism can be used by application developers to specifically filter individual contributions from Theia extensions they have no control over, i.e. core or 3rd party extensions.
Resolves #9069
Contributed on behalf of STMicroelectronics
Signed-off-by: Tobias Ortmayr tortmayr@eclipsesource.com
How to test
ContributionFilter
in an example Theia applicationReview checklist
Reminder for reviewers