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

Improvements to JProxy #470

Merged
merged 6 commits into from Jun 17, 2019
Merged

Improvements to JProxy #470

merged 6 commits into from Jun 17, 2019

Conversation

Thrameos
Copy link
Contributor

@Thrameos Thrameos commented Jun 7, 2019

Fixes the problem reported by the user and give the test bench some TLC as this and a bunch of other things were being missed. There were a lot of cases in this area where one form or another was not tested. Fixing it took much longer than expected because all of the corner cases (must be list[str or JClass], or str, or JClass, but all must resolve to JInterface).

Note. This does make the change that both bare lists and defined lists are accepted in the old JProxy interface.

JProxy([intf1, intf2], dict={...})
JProxy(intf1, intf2, dict={...})

I wanted the new interface to be simple (though specifying a list explicitly is also accepted)

   Java:  
        public class Foo implements MyInterface1, MyInterface2
   Python (preferred): 
        @JImplements(MyInterface1, MyInterface2)  class Foo
   Python (acceptable): 
        @JImplements([MyInterface1, MyInterface2])  class Foo

I can kill the second form if it is too much.

This change may break the total nonsense form JProxy(intf1, foo) where dict is being implied as based on position, which should never have worked.

A secondary change is it now reports an error on the old JProxy if either no interfaces are specified or the dict or inst is missing. Letting it quietly pass by this sort of error just to blow up later offends me.

The error message also became more terse. Giving the correct usage is the point of the documentation
and not the exception string. I added checks to the test bench to make sure the error was reported for the right reason rather than any reason. There needs to be an action item to do this universally, as people do get burned by deceptive messages.

@marscher general question, do you prefer many small tests that test each aspect of a function individually and thus report the exact failure by the test that failed, or large tests that test multiple aspects of a function? My personal preference is many small because otherwise I get burned when I fix something in a test and it just falls through the next part of the test. But I will defer to you as we are already over 400 tests.

@Thrameos Thrameos requested a review from marscher June 7, 2019 23:32
@Thrameos Thrameos self-assigned this Jun 7, 2019
@Thrameos Thrameos added this to the JPype 0.7 milestone Jun 7, 2019
@Thrameos Thrameos added the bug Unable to deliver desired behavior (crash, fail, untested) label Jun 7, 2019
@marscher
Copy link
Member

Regarding the test organization question, I'd also prefer small tests (actually as small as possible) to keep things tidied up.

@Thrameos
Copy link
Contributor Author

Due to compatibility issues with Python 2.7 JProxy only takes the form JProxy([intf1, intf2]) when multiple interfaces are specified. JImplements takes only the form @JImplements(intf1, intf2). Most is bug fix with the exception of the error message change.

@marscher Post this pull request a memory leak was discovered proxy implementation, but it will require a significant pull request to correct it. None of the JProxy code checks for a cyclic dependency in the instance. As there is no GC methods on that class there is no way that it can ever clean up if the Proxy is referred to either in the inst, dict, or Implements forms. I can try to clean up the problem as I worked through the issue in JPype 0.8 core. But it will take me at least a day or two to finish it.

@Thrameos
Copy link
Contributor Author

@marscher Okay I fixed the memory issue and backported all the relevant bug fixes from the 0.8 branch. This should fix #482 . Assuming this compiles, this and the string fix are all that are needed to release 0.7.0. You need to give me some direction on the other pull.

@Thrameos
Copy link
Contributor Author

Merging this so that the string pull can be made fresh.

@Thrameos Thrameos merged commit bdd2059 into jpype-project:master Jun 17, 2019
@Thrameos Thrameos deleted the proxy_fix branch June 17, 2019 20:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Unable to deliver desired behavior (crash, fail, untested)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants