[GitHub] commons-lang issue #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
No worries – thanks for the information.

 

From: Pascal Schumacher [mailto:notificati...@github.com] 
Sent: Saturday, October 22, 2016 9:49 AM
To: apache/commons-lang 
Cc: Derek C. Ashmore ; Mention 

Subject: Re: [apache/commons-lang] Lang 1195: Enhance MethodUtils to allow 
invocation of private methods (#141)

 

@Derek-Ashmore   Just today I learned 
that the resetting of the original accessibility of the method I asked you to 
add is not necessary, because Method#setAccessible only modifies the behavior 
of the AccessibleObject not of the actual method. 

See 
http://stackoverflow.com/questions/10638826/java-reflection-impact-of-setaccessibletrue
 and http://www.artima.com/weblogs/viewpost.jsp?thread=164042 for details.

Sorry that I caused you extra work. :(

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub 
 , or 
mute the thread 

 .  

 




---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
@Derek-Ashmore Just today I learned that the resetting of the original 
accessibility of the method I asked you to add is not necessary, because 
`Method#setAccessible` only modifies the behavior of the `AccessibleObject` not 
of the actual method. 

See 
http://stackoverflow.com/questions/10638826/java-reflection-impact-of-setaccessibletrue
 and http://www.artima.com/weblogs/viewpost.jsp?thread=164042 for details.

Sorry that I caused you extra work. :(


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Thank you! Without your pull request suggestion and additional support, it 
never would have gone anywhere. Please let me know if I can ever return the 
favor.



On Jun 5, 2016, 1:57 PM, at 1:57 PM, Pascal Schumacher 
 wrote:
>Thanks! :+1:
>
>---
>You are receiving this because you authored the thread.
>Reply to this email directly or view it on GitHub:
>https://github.com/apache/commons-lang/pull/141#issuecomment-223830424



---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Thanks! :+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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
You are right. I missed that. It is impossible to use the parameter order I 
suggested, because it would clash with the existing `invokeMethod` signature. 
Sorry.


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
I originally thought the same thing.  The problems is the "Object... args" 
support.  The fact that it's optional means that you can put an optional 
boolean at the end easily and tell the difference between its intended use to 
force access or as an argument to the method.  Fields don't have args, so 
there's no ... support. See example below. 

invokeMethod(final Object object, final String methodName, Object... args)
invokeMethod(final Object object, final boolean forceAccess, final String 
methodName, Object... args)

We *could* put it at the end in the example below, but then it's 
inconsistent with the other overload:
invokeMethod(final Object object, final boolean forceAccess, final String 
methodName, Object[] args, Class[] parameterTypes)

Am I missing something obvious?


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Hi Derek,

sorry for the delay.

I took another look at the code and compared it with `FieldUitls`. Maybe it 
is a good idea to change the method parameter order to `Object object, String 
methodName, boolean forceAccess`. I think `object` and `methodName` belong 
together and this order is similar to the order of the parameters of 
`FieldUtils` methods e.g. `FieldUtils#readDeclaredField(Object target, String 
fieldName, boolean forceAccess)`.

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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  

[![Coverage 
Status](https://coveralls.io/builds/6464031/badge)](https://coveralls.io/builds/6464031)

Coverage increased (+0.02%) to 93.428% when pulling 
**6da3ccb703cdd3539a7ffdfd168a9cfeb25689a7 on Force66:LANG-1195** into 
**43a9bab8c010d66744ae02b2d26020a946235202 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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
It's the Objects.deepEquals() -- to new for the supported jdk versions -- 
I'll 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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Good idea -- the change has been made and checked in.  Any other thoughts?


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Hi Derek,

the rebase is perfect. :+1: 

I think resetting the accessibility of should be done in a finally block, 
so that it works even when the method invocation throws an exception. 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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Hi Pascal,

I'm not sure I did the re-base part correctly.  Both my forked master and 
LANG-1195 have all changes we discussed and are based on the updated master.  
All conflicts were resolved and "I think"; you've obviously done this more than 
I have and I trust your judgement.  Please let me know if you see problems with 
what I've done.  Thanks for looking at this.


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Hi Derek,

you should be able to rebase this pull request by following these steps:
* `git remote add upstream https://github.com/apache/commons-lang.git`
* `git checkout master`
* `git pull --rebase upstream`
* `git checkout LANG-1195`
* `git rebase master`
* edit/resolve conflicts
* `git rebase --continue`
* `git push -f`

Now this pull request is rebased onto 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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Agreed -- in addition to the other changes you mentioned, I'll re-base 
using the current master so that it's easier to merge in.   Thanks for looking 
at this.


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[PascalSchumacher]

Github user PascalSchumacher commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
Thanks for updating the pull request. :+1: 

It would be nice if you could rebase on current master.

Thanks,
Pascal


---
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 #141: Lang 1195: Enhance MethodUtils to allow invocation ...

[Derek-Ashmore]

Github user Derek-Ashmore commented on the issue:

https://github.com/apache/commons-lang/pull/141
  
I happened to notice that I inadvertently made unwanted formatting changes 
in a previous commit. I've take pains to reduce the number of changed lines 
dramatically. Hopefully, this will make it easier to merge this change into the 
trunk if it's decided that this is a reasonable enhancement.

I'm also willing to make additional changes to the if you would like; 
anything I can do to make this contribution easier to merge in. 

Please let me know if you have questions.


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