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

Add IRepository.contains(T) method #314

Merged
merged 2 commits into from
Sep 14, 2023

Conversation

HannesWell
Copy link
Member

This methods provides a more convenient and in most cases faster way to test if a repository contains an element, compared to querying the repository for it.
IArtifactRepository already has a contains() method so this effectively only adds it for IMetadataRepository and generalizes it.

The implementation is based on the query() or everthing() implementation of the corresponding class, if possible.

The first of the two commits contains the clean-ups in the the IRepository and IArtifactRepository interfaces.

@github-actions
Copy link

github-actions bot commented Sep 3, 2023

Test Results

       9 files  ±0         9 suites  ±0   42m 48s ⏱️ + 3m 51s
2 175 tests ±0  2 171 ✔️ ±0    4 💤 ±0  0 ±0 
6 615 runs  ±0  6 604 ✔️ ±0  11 💤 ±0  0 ±0 

Results for commit 3436ab4. ± Comparison against base commit f891c9f.

♻️ This comment has been updated with latest results.

Copy link
Contributor

@merks merks left a comment

Choose a reason for hiding this comment

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

Looks fine.

* @return true if the given element is already in this repository
* @since 2.8
*/
boolean contains(T element);
Copy link
Member

Choose a reason for hiding this comment

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

could this be default implemented?

Copy link
Contributor

Choose a reason for hiding this comment

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

An "everything" query can be like this

new ExpressionMatchQuery<>(IInstallableUnit.class, ExpressionUtil.TRUE_EXPRESSION)

so I think it would be tricky to create a correct query even for that case though I suppose with some instanceof tests one could do that...

Copy link
Member

Choose a reason for hiding this comment

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

what about

query(QueryUtil.createMatchQuery(ExpressionUtil.TRUE_EXPRESSION)).toSet().contains(element)

Copy link
Contributor

Choose a reason for hiding this comment

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

See org.eclipse.equinox.p2.query.QueryUtil.ALL_UNITS and note the type of that query:

IQuery<IInstallableUnit> ALL_UNITS = QueryUtil.createMatchQuery(ExpressionUtil.TRUE_EXPRESSION);

Copy link
Member Author

Choose a reason for hiding this comment

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

The problem is that here it is operated on the generic type T and not IInstallableUnit and I didn't found a way to create a query for any type (generic type probably won't work since that info is lost at runtime).

But it could be default implemented in the IMetadataRepository.

Copy link
Member Author

Choose a reason for hiding this comment

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

I also tought about moving contains() to queryable but discarded it due to the high implementation work.

But with a good default impl this would be much less work and it can be overriden where suitable.
I'll try it this evening.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added it now and also provided more direct implementations in most complementing classes.

What do you think about keeping the contains(boolean) override in the IRepository in order to provide a bit better javadoc?
Unfortunately we cannot remove the contains method from the IArtifactRepository interface since this would be a binary incompatible change.

Copy link
Member

Choose a reason for hiding this comment

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

Looks good to me if it helps to have better javadoc why not, but I think you need to default implement (and delegate to IQueryable impl) this as well as otherwhise you remove the default impl inherited from the parent otherwhise.

Copy link
Contributor

Choose a reason for hiding this comment

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

It also should have an @Override if it's already declared in the base interface. And it would seem odd to add a method to API without an @since; I don't think I've ever seen an @since that includes a micro version. I mention this because of the other comment about there being no additional API; perhaps/probably a default method would additional API.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks good to me if it helps to have better javadoc why not, but I think you need to default implement (and delegate to IQueryable impl) this as well as otherwhise you remove the default impl inherited from the parent otherwhise.

Good point. Done that.

It also should have an @Override if it's already declared in the base interface.

Yes that, that's right as well. Done, too.

And it would seem odd to add a method to API without an @since; I don't think I've ever seen an @since that includes a micro version. I mention this because of the other comment about there being no additional API; perhaps/probably a default method would additional API.

Agree on that, lets see if the API tools are now fine with it.

@@ -2,7 +2,7 @@ Manifest-Version: 1.0
Bundle-ManifestVersion: 2
Bundle-Name: %pluginName
Bundle-SymbolicName: org.eclipse.equinox.p2.repository;singleton:=true
Bundle-Version: 2.7.100.qualifier
Bundle-Version: 2.8.0.qualifier
Copy link
Member

Choose a reason for hiding this comment

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

API tools now assumes no new API here so 2.7.200 seems sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe this assumption will change if/when a default method is implemented...

@HannesWell HannesWell force-pushed the repoContains branch 9 times, most recently from 964aed7 to 1944cf6 Compare September 12, 2023 21:57
@HannesWell
Copy link
Member Author

The build is now all-green.

For me this is ready.
@merks, @laeubi any final words?

This methods provides a more convenient and in most cases faster way to
test if a queryable (e.g. a repository) contains an element, compared to
querying the queryable/repository for it.
IArtifactRepository already has a contains() method so this effectively
only adds it for IMetadataRepository and generalizes it for all
IQueryable sub-types.

Co-authored-by: Christoph Läubrich <laeubi@laeubi-soft.de>
@HannesWell HannesWell merged commit c43af59 into eclipse-equinox:master Sep 14, 2023
7 checks passed
@HannesWell HannesWell deleted the repoContains branch September 14, 2023 20:57
@iloveeclipse
Copy link
Member

See #320.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants