Add custom TypeAdapter for Java models that will allow for setting _bits field #177
Conversation
…its field Without this, models created from GSON won't have their isVARSet() methods working properly.
There was a problem hiding this comment.
@RicoYao - The changes for the TypeAdapter and additional DSL work in JavaIR look good! I'm mostly just concerned around the rendering changes when using --> since it seems a bit more verbose and potentially harder to follow. Thoughts?
| }) | ||
| } | ||
|
|
||
| if !innerClasses.isEmpty { |
There was a problem hiding this comment.
It seems like the issue with a lot of the formatting here is just handling empty scopes? My only worry is that I feel like the code has becomes a bit less readable / maintainable after these changes.
There was a problem hiding this comment.
It was to handle a few things:
- Avoid having an empty line if the enums/properties/methods/innerClasses is empty.
- Avoid having tabbed empty lines. I think I can solve this in a different way by tweaking the indent() string extension function.
- Other little tweaks like adding an empty line in between each method.
I tried to simplify it a bit. Let me know if you still think it's too un-readable.
There was a problem hiding this comment.
by tweaking the indent() string extension function.
Ended up tweaking --> instead of indent()
There was a problem hiding this comment.
Thank you, this looks a lot better! Did you get a chance to run make integration_test after? I think this might alter the output of the other languages
Sources/Core/JavaModelRenderer.swift
Outdated
| ] } | ||
|
|
||
| let read = JavaIR.method( | ||
| annotations: ["Override"], |
There was a problem hiding this comment.
Question: Should we create an enum represent the various valid annotations?
There was a problem hiding this comment.
I plan to add the ability for custom annotations to be passed in through a command-line option / auxillary schema file so I don't think I can restrict it to an enum.
There was a problem hiding this comment.
So a swift enum with a associated type could work here
Something like this:
enum JavaAnnotation {
case override
case nonnull
.....
case custom(name: String, fromPackage: String) // Needed for imports?
}
Then the ones passed in via the CLI can be represented using the custom case
Before working on a PR for this would you mind putting up an RFC around the use case and general approach you'd like to take? I have some thoughts on adding more customization points to Plank and we might be able to collaborate on a solution
There was a problem hiding this comment.
Sure, I'll sync up with you on custom annotations before I add any code for that.
In the meantime, I've switched to an enum to lock it down to allowed annotations as you suggested.
Good call. I hopefully made it a bit more readable. Let me know. |
| }) | ||
| } | ||
|
|
||
| if !innerClasses.isEmpty { |
There was a problem hiding this comment.
Thank you, this looks a lot better! Did you get a chance to run make integration_test after? I think this might alter the output of the other languages
Yes, I have been running |
| let modifiers: JavaModifier | ||
| let body: [String] | ||
| let signature: String | ||
|
|
||
| func render() -> [String] { | ||
| // HACK: We should actually have an enum / optionset that we can check for abstract, static, ... | ||
| let annotationLines = annotations.map { "@\($0)" } | ||
| let annotationLines = annotations.map { "@\($0.rawValue)" } |
There was a problem hiding this comment.
I think you'll need to sort the annotations to prevent the output from becoming non-deterministic
There was a problem hiding this comment.
The output stability test should fail if I'm right on this one
There was a problem hiding this comment.
Wait you didn't actually change that so if this was going to fail it would have already...
| @@ -35,7 +35,7 @@ public struct JavaModelRenderer: JavaFileRenderer { | |||
| param.snakeCaseToPropertyName() | |||
| }.joined(separator: ",\n") | |||
|
|
|||
| return JavaIR.method(annotations: ["Override"], [.public], "int hashCode()") { [ | |||
| return JavaIR.method(annotations: [JavaAnnotation.override], [.public], "int hashCode()") { [ | |||
There was a problem hiding this comment.
you should be able to remove JavaAnnotation and just put .override here and elsewhere. only reason to do this is if the compiler is unable to infer the value types
There was a problem hiding this comment.
This can be updated in a followup PR so we can merge this
There was a problem hiding this comment.
Ok will fix in a separate PR. I also realized I missed converting the string-based annotations in JavaIR.Property so I'll fix that too.
Without this, models created from GSON won't have their isVARSet() methods working properly.
Unfortunately it's not possible to configure GSON to call property setters during deserialization (it uses reflection instead) so we need a custom TypeAdapter.
Also cleaned up some tabbing and spacing issues.