Modernize pde.project description interfaces

[HannesWell]

Based on the discussion in #1340, this is a draft to explore a possible modernization of the pde.project description API interfaces. The main changes are:

1. Rename getters starting with getX() by record-component style methods just naming what is returned (i.e. X())
2. Use Lists instead of Arrays

This allows to simplify the implementations of these interfaces to be simple records where the required methods are implemented implicitly, which is also used in this PR.

Because point one does not have a great benefit for consumers, the existing methods are kept as deprecated (but not for removal) so consumers are not forced to update.

While the new methods are in a nice and modern style, I'm not sure if the change is worth the effort for consumers.
Instead of going the full path, we could also only apply parts of this. For example only use records as implementations and implement the getters explicitly and only do the Array-to-List method change.

[github-actions]

Test Results

   285 files  ±0     285 suites  ±0   49m 15s ⏱️ - 1m 14s
 3 580 tests ±0   3 504 ✅ ±0   76 💤 ±0  0 ❌ ±0 
10 932 runs  ±0  10 701 ✅ ±0  231 💤 ±0  0 ❌ ±0 

Results for commit 61da170. ± Comparison against base commit 22f953a.

♻️ This comment has been updated with latest results.

[laeubi]

Suggested change

	List<String> friends();
	Stream<String> friends();

I usually prefer to return streams instead of collections, this has many benefits:

1. One has not to define if collection is modifiable or not
2. No need to think about the actual collection type consumers can collect to any type they want
3. easier to use in streaming / filtering if the caller further wants to customize

[ruspl-afed]

The downside of Stream for API signature is its stateful nature, callers should be ready to it.

[laeubi]

Of course each call must return a new stream that is not shared with others...

[ruspl-afed]

Yes, sure.
I rather mean that callers should be aware that they need to use it much more carefully than collection: do not try to call terminal operation more than once and so on. And this constraint may require to change programming style a bit.
Personally I'm fine with it, but the rest of PDE code base uses an ancient style full of static procedures, local variables and highly mutable data structures.

[laeubi]

We try to getting better every day :-)

[merks]

I'm personally not a fan of mixing all kinds of styles. That's just my opinion...

[HannesWell]

While Streams are often useful and suitable, e.g. in #1163, I'm not convinced they suite here because this interface models a package export. From that POV the value of the x-friends directive is a List. And these model objects are also created from a List, so I think it is fine to model it as a list again.

1. One has not to define if collection is modifiable or not

With modern Java one should not assume to get mutable collections unless it is explicitly stated by the doc. In this consumers will notice it latest on their first attempt that the returned list is immutable. But if you think it's necessary to document it, I can add it.

2. No need to think about the actual collection type consumers can collect to any type they want

As described above, in this case I think this is implied by what is modeled.

3. easier to use in streaming / filtering if the caller further wants to customize

A stream is just one .stream() away and the creation of other collection types is also easy since most collections provide a constructor that accepts arbitrary collection types.

[laeubi]

From that POV the value of the x-friends directive is a List

Really? Does order matter? Do we need to access them by index? Are duplicate entries allowed? Why then not keep String[]? If not at least the return type should be Collection

A stream is just one .stream() away and the creation of other collection types is also easy since most collections provide a constructor that accepts arbitrary collection types.

The point is that usually inside such methods one uses stream and then just collect them into a List and the caller is then converting the collected list into a stream just to do something with it and collect it into some other kind of collection (or a new list).

Also Streams are lazy evaluate, so if I'm only interested in the first element I only need to possibly construct one element while when returning a collection I always need to collect / construct the whole collection.

This does not apply here but should be kept in mind when modernizing APIs, a stream is often the better choice on the long run.

[HannesWell]

From that POV the value of the x-friends directive is a List

Really? Does order matter? Do we need to access them by index? Are duplicate entries allowed? Why then not keep String[]? If not at least the return type should be Collection

OK, I was just thinking about the directive but in the PDE model duplicates are indeed not permitted. In the UI the friends are even always sorted. So it is actually a Set and could even be a SortedSet, although the friends values are not persisted in sorted order in the manifest.

Arrays break the immutability and don't play well with recoreds since the default equals for arrays is an identity check.

A stream is just one .stream() away and the creation of other collection types is also easy since most collections provide a constructor that accepts arbitrary collection types.

The point is that usually inside such methods one uses stream and then just collect them into a List and the caller is then converting the collected list into a stream just to do something with it and collect it into some other kind of collection (or a new list).

Also Streams are lazy evaluate, so if I'm only interested in the first element I only need to possibly construct one element while when returning a collection I always need to collect / construct the whole collection.

This does not apply here but should be kept in mind when modernizing APIs, a stream is often the better choice on the long run.

I think that the Stream API is a great 'addition/extension' of the Collections API, but I don't expect that it will entirely replace collections. The point for me is, that we have a backing collection for the friends (as we can obviously not use a stream). Therefore I don't see a problem to choose its type wisely and expose the suitable type in the API. If that's sufficient for a caller: great. And if not the caller still can transform it. Always creating a stream is just a waste of resources for those that are fine with the chosen type, as we don't perform any transformation here and perform some lazy operation.

[HannesWell]

While the new methods are in a nice and modern style, I'm not sure if the change is worth the effort for consumers.
Instead of going the full path, we could also only apply parts of this. For example only use records as implementations and implement the getters explicitly and only do the Array-to-List method change.

For IHostDescription, IPackageImportDescription and IRequiredBundleDescription we have a necessary change, i.e. replacing the API using the equinox-resolver. And for IPackageExportDescription we have a change that improves the API expressiveness by replacing getFriends() that returns an array by friends() that returns a SortedSet and also avoids a copy on each call.
Consequently the change of getName() to name() as a API alignment change does not stand alone in any of the affected class and therefore seems to be acceptable for me. Especially since the 'aligned' methods are only deprecated, but not for removal. Existing clients can continue to use that method. If we want to get rid of these methods in the future we can mark them as deprecated-for-removal then.

Is everyone fine with this change or do you have concerns?

[vogella]

Great to see records in usage

[ruspl-afed]

I have a question. What is less painful: add new methods to an existing interface and mark old methods for deletion (and we will have a mix of styles for some time) or introduce new interface for modern style?
More context for my question: since records are in use (thanks!) we have new IBundleProjectService method that just constructs a record instance for us. Moreover, may be we can just forget about old style IPackageExportDescription and use PackageExportDescription record for new API.
IMO a record for API signature is no worse than, say, String.

[HannesWell]

That's a very good question I also asked myself already (sorry for the late reply).

In general I like records very much and they will become even more powerful with record patterns JEP 440 and 'withers' (to be delivered).
But especially record patterns make records a difficult choice for API elements because, as far as I know, one cannot even add a component without breaking existing patterns matching against that record type.
At least when attempting to compile a record pattern where e.g. the last component is missing ECJ emits an error (although the message sounds like one can also make it work?):

Does anyone of you knows how binary compatibility plays into this? Does the pattern not match anymore in a previously compiled class or is some kind of Error thrown?

Therefore it looks like a record as API element cannot be changed at all without potentially breaking consumers. And although the elements we model didn't change much in the past I would still like to be able to add elements without breaking consumers immediately.

So unless somebody tell's me that my previous statements are wrong I think we have to continue this path.

Unless there are objections I plan to submit this by the end of this week (around Friday).

[ruspl-afed]

I was not aware of these limitations of the record usage, so we can leave classic interface for API to avoid modern troubles.

[HannesWell]

I was not aware of these limitations of the record usage, so we can leave classic interface for API to avoid modern troubles.

Yes.

Since there was now enough time for comments and threads seem to be resolved, lets submit this.
Thanks everyone for the participation!