[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-08-07 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
@nlalevee, we already broke a few things by going Java 7 (and gained native 
symlinks 😉). For more examples, look at commons-lang3 breaking off 
commons-text and introducing escapeHtml3 and escapeHtml4 instead of escapeHtml 
-- all without changing a major version. Pointing to some hypothetical closed 
code is a poor excuse -- as if commercial vendors (or their customers) have no 
QA and expect the dependencies to be drop-ins every time. I will present the 
argument on ant-dev in a more coherent form later.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-08-06 Thread nlalevee
Github user nlalevee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
@twogee, you have looked into the open sources softwares that use Ivy, but 
Ivy is under the ASL, not the GPL, it might be used in some closed, commercial 
products :)
So we cannot know for sure that we won't break softwares by changing that 
part of the API.

The hierarchy of the classes of the DependencyResolver is not of the best 
design, it would have been great to have more composition than inheritance. But 
that's we have. So unless we want to break things, rewrite things and make an 
Ivy3, I think we should stick with it.

To move forward, I suggest that this PR doesn't break the API at all. And 
if you still think DependencyResolver deserve a probably-safe API break, you're 
welcomed to discuss it on ant-dev so we can get a consensus.



---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-08-02 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
Thanks for clarifying.

My point was that the use of the method is limited to two classes: 
`SearchEngine` and `ChainResolver` (because it is essentially a proxy).

A quick search seems to indicate that most (if not all) custom resolver 
implementations out there extend `AbstractResolver`, and thus providing a 
default implementation of the method with the new signature as a wrapper in the 
abstract class will mitigate the problems due to the lack of it.

Therefore I would like insist on adding the method to the interface as well 
as changing the signature in all resolvers that ship with Ivy. Any public 
method in the abstract class should be declared in the interface, and going 
halfway does not make much sense.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-08-02 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
Hi Gintas,

To be clear - what I meant/proposed in the dev list was:

 - It's fine to have all the changes related to introducing that new method 
with Java generics on the implementation classes, including the 
`AbstractResolver`. No objections to it.

 - However, I think we shouldn't yet introduce it on the 
`DependencyResolver` interface in this release. 

 - Finally, as you note in the latest comment, we would need to do a type 
check in our (internal) classes where we use the `DependencyResolver` 
interface's `listTokenValues` method to see if it's of type `AbstractResolver` 
and if it is, then invoke the new generic method. Else invoke the older method. 
We don't need to do a reflection check to see whether the resolver has the 
generic listTokenValues method, since the other existing method should just 
function as normal. So ultimately something like:
```
if (resolver instanceof AbstractResolver) {
   Set> tokenVals = resolver.listTokenValues(...) 
// the new generic method
} else {
   // call the method that's exposed via the DependencyResolver 
interface - the one which doesn't use generics
  Map[] tokenVals = resolver.listTokenValues(...);
}
```
In short, in this release, don't expose this as a new API on the interface 
but expose it on the `AbstractResolver` with an internal implementation that 
will trickle down to all external resolvers that extend the `AbstractResolver`. 

Again, I'm being a bit extra cautious on this specific interface unlike 
some other interface/class changes that we have been doing since this one acts 
as the central well known contract to the developers who have extended Ivy. 
Plus the fact that we do have a way to do all the changes being proposed in 
this PR without having to force the implementations of the DependencyResolver 
to implement that new method in this specific release.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
Is this sufficient, or should I add an extra check in `ChainResolver`

```
if (resolver instanceof AbstractResolver || 
hasGenericMethod(resolver)) {
...
}


   private boolean hasGenericMethod(DependencyResolver resolver) {
try {
return resolver.getClass().getDeclaredMethod("listTokenValues", 
Set.class, Map.class).getReturnType().getSimpleName().equals("Set");
} catch (NoSuchMethodException e) {
return false;
}
}
```




---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
The same applies to `ChainResolver` (which should check resolver type as 
suggested by @jaikiran); actually, all custom resolvers that do not extend 
`AbstractResolver` could be wrapped in chain resolver then as a workaround.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
The methods in `AbstractResolver` must be defined the other way around: the 
default implementation of the method with new signature should call the method 
with the old signature. May I amend the commit?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
BTW, the old method is still there, the implementation is in the abstract 
class, `AbstractResolver`.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread twogee
Github user twogee commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
The new method is taken from `AbstractOSGiResolver` where a corresponding 
method is made public. All resolvers implement only the new method and the 
fallback for the deprecated method is moved to `AbstractResolver`. The new 
method is crucial for the `SearchEngine` to work properly; leave it out and you 
may as well leave out everything.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy issue #57: fix last inconsistencies in generics

2017-07-28 Thread jaikiran
Github user jaikiran commented on the issue:

https://github.com/apache/ant-ivy/pull/57
  
Overall, this looks fine to me. I just need some inputs from the rest of 
the team on one specific change, for which I have raised a dev list thread 
https://www.mail-archive.com/dev@ant.apache.org/msg46002.html


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org