[GitHub] [maven] mkarg opened a new pull request #421: Artifact.getPath() and .setPath()

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
65 messages Options
1234
Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg opened a new pull request #421: Artifact.getPath() and .setPath()

GitBox

mkarg opened a new pull request #421:
URL: https://github.com/apache/maven/pull/421


   Simplifying usage of Java NIO API (since Java 7), so the *calling* code does not need to convert to / from Path ON ITS OWN again and again.
   
   By using `default` interface methods, this should not imply any side effects to any existing code.
   


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rfscholte commented on a change in pull request #421: Artifact.getPath() and .setPath()

GitBox

rfscholte commented on a change in pull request #421:
URL: https://github.com/apache/maven/pull/421#discussion_r549377500



##########
File path: maven-artifact/src/main/java/org/apache/maven/artifact/Artifact.java
##########
@@ -86,6 +87,16 @@
 
     void setFile( File destination );
 
+    default Path getPath()
+    {
+        return this.getFile().toPath();

Review comment:
       If `getFile()` is `null`, it should not throw an NPE




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on a change in pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on a change in pull request #421:
URL: https://github.com/apache/maven/pull/421#discussion_r549391327



##########
File path: maven-artifact/src/main/java/org/apache/maven/artifact/Artifact.java
##########
@@ -86,6 +87,16 @@
 
     void setFile( File destination );
 
+    default Path getPath()
+    {
+        return this.getFile().toPath();

Review comment:
       Good catch! Just fixed it using force-push. If you prefer, I can replace by `Optional`.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751756543


   Quick side note: using a default method is a breaking change so should likely happen only in maven >= 4 (it breaks java lang proxies).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751758976


   > Quick side note: using a default method is a breaking change so should likely happen only in maven >= 4 (it breaks java lang proxies).
   
   What can break here?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg edited a comment on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg edited a comment on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751758976


   > Quick side note: using a default method is a breaking change so should likely happen only in maven >= 4 (it breaks java lang proxies).
   
   What can break here?
   
   Isn't master *only* be used for Maven >= 4 ?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751766701


   @mkarg java lang proxies don't know how to invoke the default method so you will not get the expected behavior. This is a common breaking change since default methods were introduced. Some workaround for 2 java versions can look like https://github.com/Talend/component-runtime/blob/master/component-runtime-impl/src/main/java/org/talend/sdk/component/runtime/reflect/Defaults.java#L34 but this is really a workaround. jsonp got this breakage BTW even if it had been warned :(.
   
   > Isn't master only be used for Maven >= 4 ?
   
   Yes, point is more that if kept, it must not be backported and that if for maven 4 it can be added as a not default method too ;).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751779644


   I have no strong feelings whether we should keep it as `default` methods or whether to force all implementations to adopt it on their own. Somebody has to decide.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751817795


   From a quick review it sounds hurting less to add it and enforce impl to have it (for impl using proxies it will be hurtless) and others likely use our artifact impl so sounds a bit better even if still not 100%. Just requires to add it in maven-artifact-resolver too probably. Wdyt?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751821795


   Well, in the first place it means more work for me while it is unclear how many proxies exist for `Artifact`. ;-) But if the majority wants me to change my solution to that more complex one just for the sake of proxies, I would be happy to do that. So please vote. :-)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-751833740


   Path is slower than File, it is only interesting when the filesystem can be abstracted IIRC. Also changing the internal will not help much in regards of what is done or we need to change it everywhere at the same time (to benefit from the abstraction).
   That said I understand Markus "caller need" very much so guess we should tackle that at some point.
   An alternative is to provide another resolver method which returns NioArtifacts. Here nothing would be ambiguous probably and a plugin could even test if it exists or not.
   Lastly we must start somewhere to provide a more user friendly API, if we stick to current prerequisites it will never happen ;).


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-752252406


   If the majority wants me to replace `default` in the interface by an actual implementation in `DefaultArtifact`, then I could solve it like this: Both `setField` and `getField` do store *both* variants in *two* internal fields, i. e. we have internally a `File` *and* a `Path`, so we spare the repeated conversion-on-read while only having an overhead of one additional reference. As typically read happens much more than write, this should speed up **both** code paths (the IO path and the NIO path) in an *optimal* way, as the getters always use the specific optimal field. That would satisfy @slachiewicz **and** take care of @rmannibucau's performance concerns. WDYT? Shall I change the PR in such a way? Votes please, as I like to get this done within this week.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] slachiewicz commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

slachiewicz commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-752260014


   -1
   
   Your initial proposal is good to me, no need to change anything for 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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-756912897


   So if there is nobody voting -1 I would kindly ask someone to merge this, as I am not a committer. :-)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-756929903


   Ok for me when all *our* impl will have implemented it and do not rely on the default method.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-756929903


   Ok for me when all *our* impl will have implemented it and do not rely on the default method.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-756912897


   So if there is nobody voting -1 I would kindly ask someone to merge this, as I am not a committer. :-)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-757487758


   > Ok for me when all _our_ impl will have implemented it and do not rely on the default method.
   
   Done in https://github.com/apache/maven/pull/421/commits/5c7edad17aa2bc8b545522dba275b1d0521f6948, by either implementing it or relying on the implementation of `DefaultArtifact` (not *default method*). :-)


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] mkarg commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

mkarg commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-761526196


   There are no more open requests to change anything since six days. What do I have to do next to get this PR committed actually (I am not a committer)?


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


Reply | Threaded
Open this post in threaded view
|

[GitHub] [maven] rmannibucau commented on pull request #421: Artifact.getPath() and .setPath()

GitBox
In reply to this post by GitBox

rmannibucau commented on pull request #421:
URL: https://github.com/apache/maven/pull/421#issuecomment-761526804


   @mkarg im away from keyboard for a week but probably ping dev@maven about it saying you have 2 +1, should be enough to get a merge.


----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
[hidden email]


1234