Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

getDeclaredMethods() should search super-interfaces recursively. #55

Merged

Conversation

harawata
Copy link
Contributor

This should fix #51 .

A few comments:

  • While the original code instantiated the ArrayList lazily, this PR instantiates it before starting the scan (because it's simpler). I think it's acceptable, but please let me know if it isn't.
  • There may be a room for further refactoring (collectMethods() and collectAccessors()), but it requires a bigger change.
  • Is there any guideline for the code format? XD

@YSavanier
Copy link

Thank you very much I will test your code first thing tomorrow morning 😊

@YSavanier
Copy link

YSavanier commented Sep 28, 2018

Thank you I've checked and your implementation is even better than the former one since it also get default methods of interface herited from super interfaces !!!
I look forward seeing this added to the 3.1.x branch :)

PS : BTW in your fork I noted some code not in the PR in method getReadMethod on line 2950 you replaced

        BeanInfo info = Introspector.getBeanInfo(target);
        MethodDescriptor[] methods = info.getMethodDescriptors();

by :

        Method[] methods = target.getMethods();

Is it to overcome a potential bug in Introspector since the code seems to handle just fine with your last PR ?

B.R.

@harawata
Copy link
Contributor Author

Thank you for testing, @YSavanier !

I'm not sure which PR you are referring to, but it would be #33 that replaced Introspector (not my PR).
This is my third PR and I have not modified getReadMethods() so far.

@lukaszlenart
Copy link
Collaborator

Ok, I think we are good to merge this PR? Any objections?

@YSavanier
Copy link

I'm looking forward to it being released ^^

@lukaszlenart
Copy link
Collaborator

LGTM 👍

@lukaszlenart lukaszlenart merged commit d33f7d0 into orphan-oss:master Oct 3, 2018
@lukaszlenart
Copy link
Collaborator

@harawata do you plan to cherry-pick those changes to 3.1.x branch?

@lukaszlenart
Copy link
Collaborator

3.2.7 is under way to the Central :) Thanks a lot!

@harawata
Copy link
Contributor Author

harawata commented Oct 3, 2018

Thanks for the merge and the quick release, @lukaszlenart !
I'll open another PR for the 3.1.x port tonight.

@lukaszlenart
Copy link
Collaborator

Cool, thanks a lot for your support :)

harawata added a commit to harawata/ognl that referenced this pull request Oct 3, 2018
lukaszlenart added a commit that referenced this pull request Oct 4, 2018
Backporting #55 to ognl-3-1-x branch.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Java8Test is failed
3 participants