[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[Tibor17]

Github user Tibor17 commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
Thx for your work. I will get back to you soon. Right now I am working on
branch SUREFIRE-1463 which contains few fixes. I plan to finish this,
including build script, and then I would push my changes. I will make a
code review in this PR as I do in every other before accepting. Please wait
for me meanwhile.

On Sat, Feb 10, 2018 at 1:42 AM, Bindul Bhowmik 
wrote:

> @Tibor17 
>
> I have reviewed the Maven Coordinates documentation you mentioned, and I
> can switch the order of elements in the parameter easily. However, I think
> I would disagree with requiring the version to be last element in the
> coordinates in this use case. As the functionality stands, the
> dependenciesToScan configuration does not add additional dependencies to
> the test scope of the project, it filters dependencies already added to 
the
> test scope in the project to allow for scanning test classes to run. If
> someone wants to add say a classfier or a type/packaging, requiring them
> them to also mention the version of the dependency would just make
> maintainers life harder by having another location to keep the version of
> the dependency in sync.
>
> As such I propose, we keep the version as optional and support the
> following variations of dependencies to scan:
>
>- groupId:artifactId
>- groupId:artifactId:packaging/type
>- groupId:artifactId:packaging/type::version
>- groupId:artifactId:packaging/type:classifier
>- groupId:artifactId::classifier
>- groupId:artifactId::classifier:version
>- groupId:artifactId:packaging/type:classifier:version
>
> It still maintains the same order of elements as the Maven Coordinates
> documentation in the POM, just makes the version not required to be the
> last element, or rather makes version optional.
>
> Thoughts?
>
> —
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
> 
,
> or mute the thread
> 

> .
>



-- 
Cheers
Tibor



---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[bindul]

Github user bindul commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
@Tibor17 

I have reviewed the Maven Coordinates documentation you mentioned, and I 
can switch the order of elements in the parameter easily. However, I think I 
would disagree with requiring the version to be last element in the coordinates 
in this use case. As the functionality stands, the `dependenciesToScan` 
configuration does not add additional dependencies to the test scope of the 
project, it filters dependencies already added to the test scope in the project 
to allow for scanning test classes to run. If someone wants to add say a 
classfier or a type/packaging, requiring them them to also mention the version 
of the dependency would just make maintainers life harder by having another 
location to keep the version of the dependency in sync.

As such I propose, we keep the version as optional and support the 
following variations of dependencies to scan:

- `groupId:artifactId`
- `groupId:artifactId:packaging/type`
- `groupId:artifactId:packaging/type::version`
- `groupId:artifactId:packaging/type:classifier`
- `groupId:artifactId::classifier`
- `groupId:artifactId::classifier:version`
- `groupId:artifactId:packaging/type:classifier:version`

It still maintains the same order of elements as the Maven Coordinates 
documentation in the POM, just makes the version not required to be the last 
element, or rather makes version optional.

Thoughts?


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[bindul]

Github user bindul commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
@Tibor17,

Sorry I misunderstood your first comment. Thank you again for taking the 
time to review the changes and for your patience.

I will rework the pull request using your [latest 
comment](https://github.com/apache/maven-surefire/pull/173#issuecomment-357434597)
 and the inline review comments.

I will get back to you soon. 

Bindul


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[Tibor17]

Github user Tibor17 commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
@bindul 
Then we have to support also other alternatives:
`groupId:artifactId:version`, and
`groupId:artifactId:packaging:version`.
This means you have to, first of all, count `:` and call different private 
method/matcher for count=2, 3, 4 or 5.


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[Tibor17]

Github user Tibor17 commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
@binduk
We have modify the maven coordinates to 
`groupId:artifactId:packaging:classifier:version`, see the docs:
https://maven.apache.org/pom.html#Maven_Coordinates
Basically, only the version would be determined last.


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[Tibor17]

Github user Tibor17 commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
I don't want you to remove code. I will try to explain in code.


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[bindul]

Github user bindul commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
@Tibor17 Thank you for your review.

I agree with you that `version` check is not really useful in this use 
case, I put it in there as the JIRA ticket called for GAVT support and also to 
keep the format consistent with the other places maven defines gavtc.

I am happy to remove the version check. If I do so, do you want the input 
configuration format to still be 
`groupId:artifactId[:version[:type[:classifier]]]` or would you rather it be 
`groupId:artifactId[:type[:classifier]]`.

I will amend the commit based on your response.


---

[GitHub] maven-surefire issue #173: [SUREFIRE-1004] Support full gavtc format for dep...

[Tibor17]

Github user Tibor17 commented on the issue:

https://github.com/apache/maven-surefire/pull/173
  
The comments like this `// Check version` are not needed because it's 
obvious from code.
You may remove them or extract that part to private static methods. It's up 
to you.
If you change something please `git --ammend commit`. There should be 
single commit per PR.
Good work!


---