-
Notifications
You must be signed in to change notification settings - Fork 48
Add Support for Nested Classes #115
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
Conversation
Hah! I didn't see your PR and went off and took at stab at this myself (#118). It looks like we found some of the same things (e.g., the getCanonicalName() -> getName() bit), you handled the Jar reading better, I fixed up the plugin. I'll review yours and rebase mine on top |
Sources/Java2Swift/JavaToSwift.swift
Outdated
} | ||
|
||
// Translate all of the Java classes into Swift classes. | ||
for javaClass in javaClasses { | ||
translator.startNewFile() | ||
let swiftClassDecls = translator.translateClass(javaClass) | ||
let swiftClassDecls = try! translator.translateClass(javaClass) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the be a try
rather than a try!
?
let swiftClassDecls = try! translator.translateClass(javaClass) | |
let swiftClassDecls = try translator.translateClass(javaClass) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was just following what was there before (there was a try! inside, I just moved it out). I'm fine making this a try
) | ||
} | ||
} | ||
|
||
private func names(from javaClassNameOpt: String) -> (javaClassName: String, swiftName: String) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pulling this out
@@ -265,7 +274,7 @@ extension JavaTranslator { | |||
) | |||
|
|||
if !enumConstants.isEmpty { | |||
let enumName = "\(swiftTypeName)Cases" | |||
let enumName = "\(swiftTypeName.splitSwiftTypeName().name)Cases" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be swiftInnermostTypeName
, right?
let enumName = "\(swiftTypeName.splitSwiftTypeName().name)Cases" | |
let enumName = "\(swiftInnermostTypeName)Cases" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep missed that
while let internalClass = classes.popLast() { | ||
if let internalClass { | ||
let (javaName, swiftName) = names(from: internalClass.getName()) | ||
// If we have already been through this class, don't go through it again | ||
guard translator.translatedClasses[javaName] == nil else { continue } | ||
let currentClassName = swiftName | ||
let currentSanitizedClassName = currentClassName.replacing("$", with: ".") | ||
classes.append(contentsOf: internalClass.getClasses()) | ||
translator.translatedClasses[javaName] = (currentSanitizedClassName, nil, true) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we should always recurse into nested classes. Rather, I think we should allow specification of nested classes in Java2Swift.config
, and teach the Jar walker that produces Java2Swift.config
to walk nested classes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went back and forth on this a bit. I do like the idea of recurring down nested classes, due to the fact that the majority of uses of nested classes (at least in the JDK) are from the original class (i.e. methods that return nested classes from the main class)). I'm open to either, but like this as making a more complete class translation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does make the result more complete with less work on the config file, I agree. I have two concerns, but now I'm thinking that they aren't that important and can be addressed in the future.
One is that this means there is no way to disable the generation of the Swift import of a nested class: maybe we never need that, but at least for the moment I've avoided importing some Java classes because they trip up the translation in ways I wasn't able to immediately deal with. That said, if it became a problem, we could invent a way to do this.
The other is in how the output is structured. With this automatic recursion, the emitted file Attributes.swift
now has the Attributes.Name
type in it as well, i.e., the whole nested class definition is in one file. With the explicit recursion approach, Attributes.Name
ends up in a separate file Attributes+Name.swift
. To do that, the config file needs to list out the Attributes$Name
class as its own entry so that the Java2SwiftPlugin can know all of the files that will be emitted. It's more tidy for Swift, but it's not strictly necessary... and it would be relatively straightforward in the future to support being explicit in this way in addition to the recursion that you've implemented. For example, we could say that if you have an entry in the Java2Swift.config file for a nested class, we skip that nested class in the recursion because it's been handled at the top level already.
That's all a very, very longwinded way of saying that I agree with you now and will likely implement the "explicit" mechanism I described on top of your PR. Let's merge away!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I updated #118 to build on top of this. The Jar reader now emits nested classes (which is great), so we have to deal with some nested classes being explicitly-specified in Java2Swift.config
and others that are discovered through the recursion. Thank you for working on nested classes!
extension Attributes { | ||
@JavaClass("java.util.jar.Attributes$Name") | ||
public struct Name { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a wonderful improvement, but I think it should be driven by adding
"java.util.jar.Attributes$Name" : "Attributes.Name",
to this module's Java2Swift.config
.
Closes #19
This adds support for nested classes. There are 2 parts here:
This also migrates some places where we were using
getCanonicalName
when we should be usinggetName
(to make sure that we use the $)