-
Notifications
You must be signed in to change notification settings - Fork 722
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
Strings: option to generate Objective-C compatible Swift code #490
base: stable
Are you sure you want to change the base?
Strings: option to generate Objective-C compatible Swift code #490
Conversation
Ooohh nice! We just have to check if that doesn't "pollute" the original template too much (sometimes the frontier between parametrizing a template and creating a separate one is thin…) — haven't dug into the code of the PR yet — but if not, that's a good way to keep both Swift-only and ObjC-compatible generated code in sync 👍 |
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.
Wow now that I see the changes, didn't expect it to be such a small addition to the existing template👌 Good choice making it a param instead of a separate template then 👍
Only thing that's missing is the tests for checking if the generated output compiled without errors. We have a task in our Rakefile
to compile the generated outputs using swiftc
to check there's no compile error; so for those templates I guess it would make sense to do the same with an ObjC compiler? Or is the rake task compiling those outputs with swiftc enough to spot any potential issue with the generated code from the ObjC PoV? Maybe so 🤔
PS: We actually have a project with mixed Swift/ObjC at work for which a coworker created a template, I'll try to compare what she did with what you posted here to see if there's anything missing 🙂
@@ -3,6 +3,7 @@ | |||
|
|||
{% if tables.count > 0 %} | |||
{% set accessModifier %}{% if param.publicAccess %}public{% else %}internal{% endif %}{% endset %} | |||
{% set rootTypeName %}{{param.enumName|default:"L10n"}}{% endset %} |
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.
We're on the verge of releasing 6.0 — which will be a breaking version with some changes and with people having to update their config files.
So even if we don't have time to merge this PR in 6.0 (we've already postponed 6.0 for too long so I don't want a PR to block the release if it's not ready by the time we got the release button) and only include it in 6.1, but maybe 6.0 could be the occasion to rename the param from enumName
to rootTypeName
at least, even before that PR?
@djbe thoughts? You already renamed that param for ib templates in #419 (splitting scene and segue in two separate templates), so we might just do a quick PR only to rename those before 6.0, so that when this PR lands maybe after 6.0, then at least it will not be a breaking change anymore but just an additive one?
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.
Oh didn't know about the Rakefile
. Well swiftc
should be enough to make sure that the generated source is valid, i.e. it compiles without errors.
What it will not test is that the Obj-C annotations are correct and therefore the exports to Obj-C work correctly. So that might be a possible extra test. I don't have any experience with Rake
, but could try to have a look (if none of you guys would volunteer 😀)
Renaming the enumName
in 6.0 breaking release might be nice addition if there is already breaking release coming.
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.
Yeah now that I think about it, compiling the output with swiftc
should be enough even for ObjC template. After all, we're not generating ObjC code, we're still generating Swift code, just with @objc
annotations, so 👍
What could be nice in addition to compiling the generated code to ensure it's valid Swift… would be to compile a small file with code calling that generated Swift code (so for example for each strings generated output file we could provide a .swift
file calling some of the generate constants like print(L10n.foo)
or such…). And then we could add the same type of calling code to be compile-tested but written in ObjC. That would also be a nice additional test suite to ensure that the call site still looks nice and isn't modified by any modification of our templates, for example. But as of today, we don't have that yet even for Swift — we only compile the generated output, not code calling that generated output. Might be worth a separate, dedicated PR though, since it would be an entire new class of tests, rather than trying to do that directly in the current PR?
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.
Yeah what you are describing is exactly the way I set it up for myself for development and experimenting on feasibility for this PR. I have a small project with one Swift and one Objective-C file printing various localizations using all possible ways (property, method with argument). And the fact it compiles should be good enough verification.
Btw, you only generated that parameter for the String templates, do you plan to submit other PRs to support ObjC also for other templates (storyboard scenes & segues, colors, images, …) as well in the future? |
I have created Strings for now as I needed it 😄 I am not using other templates at the moment, but if you think that would make a good addition, I might have a look in coming days (or rather weeks, depends on how it would go), if no one will pick it up before. |
Sure, no pressure. Btw if you need push access to SwiftGen (to avoid doing forks and be able to create branch directly in the repo for an easier contributing experience) don't hesitate to lmk |
{% if param.objcCompatible %} | ||
@objcMembers @objc({{rootTypeName}}{{typeName}}) | ||
{{accessModifier}} final class {{typeName}}: NSObject { | ||
{% else %} | ||
{{accessModifier}} enum {{table.name|swiftIdentifier:"pretty"|escapeReservedKeywords}} { |
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 should also use typeName
. I'll fix that.
501d6ec
to
cc6f4ad
Compare
Fixed minor PR issue and resolved all conflicts with latest state of |
@djbe I don't want to make the mistake to "one more thing, just take another last-minute PR for 6.0" and merge too fast while we're so close to 6.0, but given this PR changes the |
@AliSoftware just a reminder: In current state this PR does not change param name from the outside. As we discussed before, it keeps the old param name |
The thing is, we have 2 Obj-C PRs at this moment, and I'm not sure which is the best way to go.
Normally, I'd say keep it simple (so this one), but I'm wary of adding unrelated stuff to our existing templates. And like @lukaskukacka mentioned, we can keep things backwards-compatible quite easily. |
Ok. Right. It doesn't seem ready anyway (in the sense that it passes the CI sure but might miss more docs, decision on merging it alongside the other ObjC PR or not, and changing So let's wait for after 6.0 anyway then I guess. We could then merge this later in 6.1, even deprecating So agreed, let's not rush it, and be zen while doing the long-overdue 6.0 release this weekend and review that after that big step is passed 😉 |
@AliSoftware @lukaskukacka hey guys, is there any chance to merge this in the nearest future? |
@ismetanin I wish, but not in my powers in the moment. We need feedback from maintainers. @AliSoftware @djbe did you finally make a decision if you will use my solution? Are there any changes needed? Or did you decide to take a different way? |
We haven't taken any decision yet on this, and I have to admit I haven't had much time lately to do much maintenance, so this one is still pending. Given that this PR is still not finished (especially missing documentation update for the new parameter) and thus not ready to be merged, you might as well instead use the dedicated ObjC templates which are now merged (see #378). I'm not even sure if it's worth adding both ways (the dedicated ObjC templates already merged, and the param to add obj annotations like here) to generate ObjC-compatible code after all? |
@AliSoftware @djbe any decision or change here please? Nothing stops people from using own template of course (for example of copy of the one from my PR), but it might be nice to have first-class support for it. |
Agreed, is the docs the only thing missing for this? |
I would have to rebase, probably rework some parts and generally make sure it still all works as expected after 2.5y of being stale 😀 |
Hello @djbe @AliSoftware, This PR has been open 5 years ago, yet there are still mixed Obj-C/Swift projects which could benefit from it. Given I would make it up to date again, do you think it would even make it to be merged or shall I consider it completely irrelevant at this point? Thanks for having a look |
This PR resolves #489
Technical solution
This solution is 100% backwards compatible as mentioned in reported issue as a requirement.
There is an internal change to rename
enumName
variable torootTypeName
as IMHO it makes more sense in context of supporting more thenenum
. To keep backwards compatibility, the parameter nameenumName
passed toswiftgen
stays unchanged.Decisions
Swift 4
I have implemented this change just for Swift 4 as I believe it is a way forward and there is no way to support new features for old language versions.
Flat only
This change supports only flat template for simplicity. Need and possible solutions for nesting keys can be evaluated as a separate issue.