[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/11177727) Coverage increased (+0.09%) to 94.693% when pulling **6aee4203260f93cf4511779e8078952e90d6bb6f on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/11177727) Coverage increased (+0.09%) to 94.693% when pulling **6aee4203260f93cf4511779e8078952e90d6bb6f on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 @PascalSchumacher , sorry for my bad decisions. Again I thought I should keep it public to provide same functionality for someone who needs in future :) Now, I removed it to MethodUtils as a private method. I also added @since tag and mentioned interfaces in @return java doc. Thanks a lot! --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 If I'm not mistaken the list returned by `getAllSuperclassesAndInterfaces` can contain the same interface multiple times if it is implemented by more than one super class. To prevent this the result should be collected in `LinkedHashSet` first (similar to https://github.com/yasserzamani/commons-lang/blob/90e252f571377cac39f5e2a3fc73749f589c24de/src/main/java/org/apache/commons/lang3/ClassUtils.java#L448). What do you think? --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 Created https://issues.apache.org/jira/browse/LANG-1321 for the `ClassUtils#getAllSuperclassesAndInterfaces` addition. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 > Sorry for the delay, other open source projects and vacations interfered. You're welcome, I understand and as an open source fun contributor, I'm patient enough :) > I'm not sure if the priority parameter is necessary? No, is not. I just thought I should make it more general with more options to prevent future needs for other user's PRs. Now, with thanks to your recommendations I remove it :) --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/11170248) Coverage increased (+0.07%) to 94.675% when pulling **90e252f571377cac39f5e2a3fc73749f589c24de on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 @yasserzamani Sorry for the delay, other open source projects and vacations interfered. I'm not sure if the priority parameter of `getAllSuperclassesAndInterfaces` is necessary? Are you aware of any use-cases where it is necessary? Maybe we should keep things simple and remove it? Otherwise the pull request is fine. :=) :+1: --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 Concerning the coverage: I think it's because the coverage differs between java versions. The build used to determine coverage depends on the order in which the travis builds finish. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 Thank you @Abrasha , I signed up with [coveralls.io](http://coveralls.io) and see that all of this PR's changes are covered with unit tests: [MethodUtils](https://coveralls.io/jobs/24310263/source_files/797094203#L845) , [ClassUtils](https://coveralls.io/jobs/24310263/source_files/797094115#L477). Also these files show enhancement in coverage at [COVERAGE CHANGED](https://coveralls.io/jobs/24310263) :thinking: --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/10822401) Coverage decreased (-0.03%) to 94.577% when pulling **b79fe6f9e350edb3086c44220968ae3dfcc0d2cb on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user Abrasha commented on the issue: https://github.com/apache/commons-lang/pull/261 @yasserzamani I thinks because you added more conditional branches in the latest commit and that is why less LOC are covered --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 @PascalSchumacher , I made it better ð thanks a lot! Please feel free to make any recommendation if you think I can make it better. > Coverage decreased (-0.03%) to 94.571% ð Do you know why? ð --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/10806431) Coverage decreased (-0.03%) to 94.571% when pulling **7390433b9dc9f53d2cf472421fe63d8240756492 on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 ð I am working on a new commit. thanks! --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 Well we have to keep `getMethodsWithAnnotation(cls, annotationCls)` for compatibility reasons. In my opinion it is not problem if is equal to `getMethodsWithAnnotation(cls, annotationCls, false, false)`. It can be refactored to call `getMethodsWithAnnotation(cls, annotationCls, false, false)` and then it is just some sugar for the most? common use case. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue:
https://github.com/apache/commons-lang/pull/261
Another bad thing is that `getMethodsWithAnnotation(cls, annotationCls,
false, false)` will be a duplicate for `getMethodsWithAnnotation(cls,
annotationCls)`.
Another solution is to keep my current changes not modified but add
following javadoc:
```
*
* To lookup annotations on the given class level only choose {@code
get*()} methods
* and to lookup in the entire inheritance hierarchy of the given class and
ignore
* accessibility, choose {@code find*()} methods
*
* @since 2.5
*/
public class MethodUtils {
```
---
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.
---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 You are right. The method signature I suggested omits the important super part. I agree it should be something like `getMethodsWithAnnotation(Class cls, annotationCls, boolean searchSupers, boolean ignoreAccess)`. (I'm not a fan of boolean parameters (especially multiple ones), so if somebody has an idea for a good method name, that is welcome.) --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user yasserzamani commented on the issue: https://github.com/apache/commons-lang/pull/261 @PascalSchumacher , thank you! I was overloaded by another job and forgot that I also can use method overloads. Yes, strongly I agree. Just I have some doubts about name `ignoreAccessiblitiy` because the new method is not only about accessibility, but also about traverse super classes and implemented interface and super interfaces while the annotation maybe is not present on the given class but is present on lower layers. Maybe we should also add another param named `searchSupers` for example. --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user PascalSchumacher commented on the issue: https://github.com/apache/commons-lang/pull/261 Thanks for the pull request! It is really unfortunate that the existing method `getMethodsWithAnnotation` is not called `getAccessibleMethodsWithAnnotation` : (, because then the new method could be called `getMethodsWithAnnotation`. I would not have been able to tell the difference between `getMethodsWithAnnotation` and `findMethodsWithAnnotation` without reading the javadoc. I believe we need a more intention revealing method name or maybe we should just add an overload of `getMethodsWithAnnotation` e.g. `getMethodsWithAnnotation(Class cls, annotationCls, boolean ignoreAccessiblitiy)`. What do you think? --- 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. ---
[GitHub] commons-lang issue #261: LANG-1317: Add findAnnotation and findMethodsWithAn...
Github user coveralls commented on the issue: https://github.com/apache/commons-lang/pull/261 [](https://coveralls.io/builds/10775689) Coverage decreased (-0.002%) to 94.603% when pulling **e49a3a2099759f5fee2423aff2b9fa762881c36b on yasserzamani:LANG-1317** into **4a300fee2ef1c03902d0fb25ceb02aa01d0fab46 on apache:master**. --- 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. ---
