You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@maven.apache.org by GitBox <gi...@apache.org> on 2022/12/15 06:28:32 UTC
[GitHub] [maven] laeubi opened a new pull request, #913: MNG-7630 - Support listing of workspace models
laeubi opened a new pull request, #913:
URL: https://github.com/apache/maven/pull/913
... and implement upcoming interface methods of [MRESOLVER-307](https://issues.apache.org/jira/browse/MRESOLVER-307).
Currently the WorkspaceReader has a way to list versions or resolve the file for an artifact or the model, but there is no way of really get a list of workspace artifacts or models itself.
This adds ways to list artifact and models a WorkspaceReader manages.
Following this checklist to help us incorporate your
contribution quickly and easily:
- [x] Make sure there is a [JIRA issue](https://issues.apache.org/jira/browse/MNG-7630) filed
for the change (usually before you start working on it). Trivial changes like typos do not
require a JIRA issue. Your pull request should address just this issue, without
pulling in other changes.
- [x] Each commit in the pull request should have a meaningful subject line and body.
- [x] Format the pull request title like `[MNG-XXX] SUMMARY`, where you replace `MNG-XXX`
and `SUMMARY` with the appropriate JIRA issue. Best practice is to use the JIRA issue
title in the pull request title and in the first line of the commit message.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Run `mvn clean verify` to make sure basic checks pass. A more thorough check will
be performed on your pull request automatically.
- [ ] You have run the [Core IT][core-its] successfully.
If your pull request is about ~20 lines of code you don't need to sign an
[Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf) if you are unsure
please ask on the developers list.
To make clear that you license your contribution under
the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
you have to acknowledge this by using the following check-box.
- [x] I hereby declare this contribution to be licenced under the [Apache License Version 2.0, January 2004](http://www.apache.org/licenses/LICENSE-2.0)
- [ ] In any other case, please file an [Apache Individual Contributor License Agreement](https://www.apache.org/licenses/icla.pdf).
[core-its]: https://maven.apache.org/core-its/core-it-suite/
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081687942
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
> So ultimately, except the point we didn't understand each others, everything converges to a collection it seems.
It does not, implementors are free to cache items internally in what ever way they like, this must not be exposed as API, callers are also free to hold their own copy in whatever type they seems fit their needs, this must not exposed in the API. Collection return types on the other hand has major drawbacks that need to be documented and mitigated streams do not have that are:
1. Carefully document if the returned collection is the property of the caller or the callee
2. Carefully document if the returned collection is safe to be shared across threads
3. Carefully document if the returned collection is safe to be cached
4. Carefully document if it is modifiable or not (just see recent bug in 3.8.6 ([MNG-7316](https://issues.apache.org/jira/browse/MNG-7316))
And yes I proposed the API and I designed the API and no one ever cared to do so before so it was designed with what I have in mind here and like to use, collections are not, especially not just for the sake "because we always did it that way" or some strange assumtions, otherwhise all Maven methods need to throw `Exception` and return AutoClosable objects because you "never can know". Thats why we have API contracts and the Contract says that a Stream (with no particular order) must be returned, it does not claim that the Stream must be closed nor that it throws `IOException` so if *any* I/O is performed as part of the implementation it is the fully responsibility of this hypothetical implementation to take action that ensure appropriate closing of system resources is performed and I/O problems are properly handled.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081611183
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
Streams also have the con to need collection to be cached (if you call it twice for ex).
Streams also have the disavantage to not be reusable so if anyone call it and need to browse it twice it will have to materialize it too (toList()) making the call.
So at the end from a caller perspective the collection usage is lighter in terms of contract and compatible with streams (`.stream()`).
now if you explicit you must enforce close - which is the current PR contract since everything is abstracted and can be backed by a filesystem by contract - it is fine too, just not as convenient IMHO.
Key decision point for me is: does the data fit in mem: obviously yes it is in maps already, so no need to stream it IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396619075
> > > > pls rebase against latest maven-3.9.x branch
> > >
> > >
> > > Will do, btw maybe this feature can be enabled: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/ ?
> > > this will give a nice rebase button in the UI
> >
> >
> > Don't! We had it enabled and disabled on purpose because commit metadata was lost. @slawekjaranowski remembers.
>
> I use it all the time for some projects, never noticed anything was lost?!?
At least committer information was gone multiple times.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081238732
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
Why the not return a collection? A conversion to a stream is still possible.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396594967
> Will do, btw maybe this feature can be enabled: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/ ?
Unsure who has power to enable those, I cannot.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi closed pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi closed pull request #913: MNG-7630 - Support listing of workspace models
URL: https://github.com/apache/maven/pull/913
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1080930862
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,18 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
Review Comment:
since 3.9.0
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,18 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ */
+ Stream<Artifact> listArtifacts();
+
+ /**
+ * List all available models this workspace repository manages.
+ *
+ * @return a stream of models in no particular order
Review Comment:
since 3.9.0
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
michael-o commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081651908
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
Very nice and profound knowledge you both have!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1385135621
@cstamas @olamy can we include this in 3.9.0 release?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081260241
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
@laeubi would be true if you use impl and not interfaces else you can only not know and should close the stream to respect the contract. Here collection looks sufficient and simpler IMHO.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081279805
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
Well that's all a bit hypothetical.... if you are certain you can always wrap streams in try-with-resource...
Streams are much better suited than collections, as the caller can filter them or convert them to any type they need without having to think about if the collection could be modified or not.
Streams also the the advantage that they are _lazy_ if I search for an item (using `findFirst()` / `findAny()` / `anyMatch()`) only those items are evaluated up to the point where a match is found in contrast to a collection that is _greedy_ and thus requires to evaluate all content even if the first item is what I want.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081186329
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
[From the javadoc](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html):
> Streams have a [BaseStream.close()](https://docs.oracle.com/javase/8/docs/api/java/util/stream/BaseStream.html#close--) method and implement [AutoCloseable](https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html), **but nearly all stream instances do not actually need to be closed after use**. Generally, only streams whose source is an IO channel (such as those returned by [Files.lines(Path, Charset)](https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#lines-java.nio.file.Path-java.nio.charset.Charset-)) will require closing. Most streams are backed by collections, arrays, or generating functions, which require no special resource management.
as there is no IO involved, there is no problem and callers can convert to list if they think it is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396560239
Am fine with this go into 3.9 and not impose any change in resolver, and here is why:
* there is already this MavenWorkspaceReader that already extends resolver WorkspaceReader by adding "maven specific" calls onto it.
* Resolver workspace reader has all of its requirements fulfilled with existing WorkspaceReader interface, so adding methods there ONLY to fulfill some 3rd party (integrating) library is a no go for me (and just like Maven, 3rd party user can extend WorkspaceReader to suit its own needs)
* hence, doing this or similar change on resolver WorkspaceReader is IMHO no go and not needed, OTOH, this change here (in "maven realm") is completely OK to me
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081652684
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
> Caching is not a concern of the API.
iterating and reitarating over the map on big project will be a topic IMHO if these methods are useful.
> If I need to browse them twice (why?) I can again make a collection,
`I` is you in this PR? if so I agree, if the caller then no cause you can get multiple callers and would mean your API is not relevant.
> there are literally no code except the I/O cases where it is recommended
huh, no. But anyway you can't assume the related API has no IO related.
> This is obviously not a decisions point, because then most of the time no one would ever use streams as most of them "fit in memory".
Which is what is in most real not overkill/hype API ;).
> while a collection approach would require
this part is wrong, what you propose is to visit the whole stream and mutate it whereas you can push back the filters and things like that in the actual materialization stream (the impl) which is what is done at maven in general (think scope filter for ex) and is more efficient on all the points you mention.
So ultimately, except the point we didn't understand each others, everything converges to a collection it seems.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1080948867
##########
maven-core/src/main/java/org/apache/maven/internal/aether/MavenChainedWorkspaceReader.java:
##########
@@ -60,6 +62,31 @@ public Model findModel(Artifact artifact) {
return null;
}
+ @Override
+ public Stream<Artifact> listArtifacts() {
+ return Arrays.stream(readers)
+ .flatMap(r -> {
+ // TODO once resolver 2.x is there this part can be simplified
Review Comment:
eh? :smile:
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396591612
> pls rebase against latest maven-3.9.x branch
Will do, btw maybe this feature can be enabled: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/ ?
this will give a nice rebase button in the UI
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081012109
##########
maven-core/src/main/java/org/apache/maven/internal/aether/MavenChainedWorkspaceReader.java:
##########
@@ -60,6 +62,31 @@ public Model findModel(Artifact artifact) {
return null;
}
+ @Override
+ public Stream<Artifact> listArtifacts() {
+ return Arrays.stream(readers)
+ .flatMap(r -> {
+ // TODO once resolver 2.x is there this part can be simplified
+ if (r instanceof MavenWorkspaceReader) {
+ return ((MavenWorkspaceReader) r).listArtifacts();
+ }
+ return Stream.empty();
Review Comment:
maybe use `filter(it -> instanceof MavenWorkspaceReader).map(MavenWorkspaceReader.class::cast)`, will avoid the debate of `if/else` (readability on the left vs symmetry - no good choice there)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1397941596
I'll close this PR and just try to spin up my own API, even though this will replicate some code from maven and do not gracefully integrates with other IDEs.
It seams maven is not ready yet for "overkill/hype API" changes ...
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1080950099
##########
maven-core/src/main/java/org/apache/maven/internal/aether/MavenChainedWorkspaceReader.java:
##########
@@ -60,6 +62,31 @@ public Model findModel(Artifact artifact) {
return null;
}
+ @Override
+ public Stream<Artifact> listArtifacts() {
+ return Arrays.stream(readers)
+ .flatMap(r -> {
+ // TODO once resolver 2.x is there this part can be simplified
+ if (r instanceof MavenWorkspaceReader) {
+ return ((MavenWorkspaceReader) r).listArtifacts();
+ }
+ return Stream.empty();
Review Comment:
personally I prefer if-else, reading code similar to this requires more attention IMHO and is easy to slip on it
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396606402
> -P format
Would be cool to have a github action doing this and automatically push this to the branch if the "allow edit by maintainers" is enabled!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396562822
pls rebase against latest maven-3.9.x branch
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] michael-o commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
michael-o commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396607144
> > pls rebase against latest maven-3.9.x branch
>
> Will do, btw maybe this feature can be enabled: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/ ?
>
> this will give a nice rebase button in the UI
Don't! We had it enabled and disabled on purpose because commit metadata was lost. @slawekjaranowski remembers.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] cstamas commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
cstamas commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396599551
To reformat build with `-P format` and commit source changes
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081859598
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
> It does not, implementors are free to cache items internally
of reactor reader? you want to override it?
About the collection pitfalls, I guess it is mainly about being consistent with the track on the API and internals we started so I'd say:
1. make it immutable and in the javadoc,
2. solved by 1,
3. solved by 1,
4. solved by 1.
Strictly speaking streams have the exact same pitfall but you move this issue "when you materialize" the view so this is not really a point except the fact the view should be immutable as all the work done in the rest of the API probably.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on PR #913:
URL: https://github.com/apache/maven/pull/913#issuecomment-1396612596
> > > pls rebase against latest maven-3.9.x branch
> >
> >
> > Will do, btw maybe this feature can be enabled: https://github.blog/changelog/2022-02-03-more-ways-to-keep-your-pull-request-branch-up-to-date/ ?
> > this will give a nice rebase button in the UI
>
> Don't! We had it enabled and disabled on purpose because commit metadata was lost. @slawekjaranowski remembers.
I use it all the time for some projects, never noticed anything was lost?!?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081641445
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
Caching is not a concern of the API. Using streams allows implements to cache (as they can be sure no one can ever modify the underlying collection, what ever type and whatever technique). so this is really not an argument.
If I need to browse them twice (why?) I can again make a collection, I even need to do so, because I can not assume it is always stable (e.g. modified) unless I copy it (or the calling method copies it) so this is really not an advantage at all.
> So at the end from a caller perspective the collection usage is lighter in terms of contract and compatible with streams (.stream()).
I don't think so, and of course stream is compatible with collection as well (.toList()) so this also is just not really an argument.
> now if you explicit you must enforce close
Sorry the statement of that one **must** close a stream was brought up by you and I proved its wrong, the API clearly state that closing is not required except for very special (and documented) cases, and even there it is documented that one _should_ close the stream see: https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#lines-java.nio.file.Path-java.nio.charset.Charset-
> If timely disposal of file system resources is required, the try-with-resources construct should be used to ensure that the stream's [close](https://docs.oracle.com/javase/8/docs/api/java/util/stream/BaseStream.html#close--) method is invoked after the stream operations are completed.
> now if you explicit you must enforce close - which is the current PR contract since everything is abstracted and can be backed by a filesystem by contract - it is fine too, just not as convenient IMHO.
so this whole "close" story is completely unrelated here and is just confusing, there are literally no code except the I/O cases where it is recommended, and you won't find code that closes simple streams "just in case"... and even if an implementation might has requirements for close it still can collect everything, close the stream and return a "save" stream to the caller, but that's nothing an API must take care of.
> Key decision point for me is: does the data fit in mem: obviously yes it is in maps already, so no need to stream it IMHO.
This is obviously not a decisions point, because then most of the time no one would ever use streams as most of them "fit in memory". And even if the current one is held in memory this won't necessary be the case for other implementations of this and it might be costly to collect everything, it even might delay thing (just think about maven would resolve and download **all** artifacts before the progress is reported in one big bunch, even though it is same as fast, you will want a stream of messages as they happen)
Also please look at the `MavenChainedWorkspaceReader` implementation, that currently can simply return the stream from inner providers, combine them, and even can make sure they are still unique, while a collection approach would require:
- Each implementation must collect everything eager (even if not used at all)
- It must create a copy (if dynamic) or read only collection
- The chained reader must again maintain own collections and add all element it collects again, making it unique e.g. by a set (additional efforts) that includes traversing all object
- The caller most likely will not just store the collection but iterate it (foreach or stream) and thus must traverse all objects again.
Because of this, I'm strongly convinced that Streams are superior in this case (and even others), are a more modern way of "collections" and are much more flexible anyways, for everyone that is more used to collections they can easily be transformed into such.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] laeubi commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
laeubi commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081186329
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
[From the javadoc](https://docs.oracle.com/javase/8/docs/api/java/util/stream/Stream.html):
> Streams have a [BaseStream.close()](https://docs.oracle.com/javase/8/docs/api/java/util/stream/BaseStream.html#close--) method and implement [AutoCloseable](https://docs.oracle.com/javase/8/docs/api/java/lang/AutoCloseable.html), but nearly all stream instances do not actually need to be closed after use. Generally, only streams whose source is an IO channel (such as those returned by [Files.lines(Path, Charset)](https://docs.oracle.com/javase/8/docs/api/java/nio/file/Files.html#lines-java.nio.file.Path-java.nio.charset.Charset-)) will require closing. Most streams are backed by collections, arrays, or generating functions, which require no special resource management.
as there is no IO involved, there is no problem and callers can convert to list if they think it is required.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [maven] rmannibucau commented on a diff in pull request #913: MNG-7630 - Support listing of workspace models
Posted by GitBox <gi...@apache.org>.
rmannibucau commented on code in PR #913:
URL: https://github.com/apache/maven/pull/913#discussion_r1081182626
##########
maven-resolver-provider/src/main/java/org/apache/maven/repository/internal/MavenWorkspaceReader.java:
##########
@@ -28,4 +29,20 @@
public interface MavenWorkspaceReader extends WorkspaceReader {
Model findModel(Artifact artifact);
+
+ /**
+ * List all available artifacts this workspace repository manages.
+ *
+ * @return a stream of artifacts in no particular order
+ * @since 3.9.0
+ */
+ Stream<Artifact> listArtifacts();
Review Comment:
maybe it requires a list (both methods) cause in current state nobody closes the related streams so it can lead to issues (rest LGTM)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@maven.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org