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

allows static method calls on JNIObject #14

Merged
merged 5 commits into from
Aug 22, 2019
Merged

Conversation

michaelknoch
Copy link
Collaborator

No description provided.

@michaelknoch michaelknoch requested a review from ephemer August 21, 2019 16:08
try checkAndThrowOnJNIError()
self.javaClass = javaClass!
self.instance = globalInstanceRef
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good overall, but we should deprecate the current convenience init and force use of the new API where users have to override className (it's weird to have a convenience init with className here)

Copy link
Member

@ephemer ephemer Aug 21, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best way to do that would be to add

@available(*, deprecated, message: "Override Self.className instead and use the initializers that don't take className as an argument")
convenience public init(_ className: String, arguments: [JavaParameterConvertible] = []) throws {
  // deprecated init
}

// note the variadic arguments syntax instead of the array
convenience public init(...arguments: [JavaParameterConvertible]) throws {
  // use `Self.javaClass` here
}

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i like the idea and i commited the changes. One thing, Self does not seem to be available here and i used type(of: self).javaClass to workaround it. Not really understanding why

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@michaelknoch michaelknoch requested a review from ephemer August 21, 2019 17:50
@michaelknoch michaelknoch dismissed ephemer’s stale review August 21, 2019 17:55

improved a lot of stuff :)


private static var classInstances = [String: JavaClass]()

public class var javaClass: JavaClass {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can be static now I think?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, you are right

@michaelknoch
Copy link
Collaborator Author

thanks for your help & guidance @ephemer

@michaelknoch michaelknoch merged commit 83c9859 into devel Aug 22, 2019
@michaelknoch michaelknoch deleted the static-methods branch August 22, 2019 06:50
@ephemer
Copy link
Member

ephemer commented Aug 22, 2019

@michael my pleasure! Thanks for putting this PR in

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.

2 participants