Strings: option to generate Objective-C compatible Swift code #490

lukaskukacka · 2018-09-07T18:08:50Z

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 to rootTypeName as IMHO it makes more sense in context of supporting more then enum. To keep backwards compatibility, the parameter name enumName passed to swiftgen 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.

AliSoftware · 2018-09-07T18:33:58Z

Ooohh nice!
When we talked about that I thought you'd implement it by creating a brand new, separate template.
Including that in the original Swift template to allow bridging is quite clever, especially with the @objc annotations 👍

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 👍

AliSoftware

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 🙂

AliSoftware · 2018-09-07T18:42:24Z

templates/strings/flat-swift4.stencil

@@ -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 %}


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?

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.

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?

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.

AliSoftware · 2018-09-07T19:24:32Z

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?

lukaskukacka · 2018-09-07T19:37:33Z

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.
Feel free to create an issue and I'll pick it up 😎

AliSoftware · 2018-09-07T19:55:09Z

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

lukaskukacka · 2018-09-12T09:12:13Z

templates/strings/flat-swift4.stencil

+  {% if param.objcCompatible %}
+  @objcMembers @objc({{rootTypeName}}{{typeName}})
+  {{accessModifier}} final class {{typeName}}: NSObject {
+  {% else %}
  {{accessModifier}} enum {{table.name|swiftIdentifier:"pretty"|escapeReservedKeywords}} {


This should also use typeName. I'll fix that.

…s Swift code

…rings

lukaskukacka · 2018-09-28T18:14:25Z

Fixed minor PR issue and resolved all conflicts with latest state of master branch.

AliSoftware · 2018-09-28T18:18:01Z

@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 param enumName to rootTypeName — which makes sense to be more flexible — and 6.0 will be breaking, should we indeed merge that last-minute too?

lukaskukacka · 2018-09-28T18:21:27Z

@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 enumName. The name is changed just internally. We would have to make more changes to this PR if the param name would be removed.

djbe · 2018-09-28T18:23:11Z

The thing is, we have 2 Obj-C PRs at this moment, and I'm not sure which is the best way to go.

This is the "simpler" PR in that it adds some things to existing templates
The other is a full ObjC template

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.

AliSoftware · 2018-09-28T18:28:38Z

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 enumName in the param on all templates if we want to finally go the breaking route in order to have consistency between the variable inside the template and the exposed param name…

So let's wait for after 6.0 anyway then I guess. We could then merge this later in 6.1, even deprecating param.enumName (but still backwards-support it, so {% set rootTypeName %}{{ param.rootTypeName|default:param.enumName|default:L10n}}{% endset %} or something) in that 6.1 and only completely drop the deprecated param.enumName in config file in 7.0…

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 😉

ismetanin · 2020-01-15T09:30:05Z

@AliSoftware @lukaskukacka hey guys, is there any chance to merge this in the nearest future?

lukaskukacka · 2020-01-15T16:49:51Z

@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?

AliSoftware · 2020-05-17T17:48:39Z

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?

lukaskukacka · 2022-12-06T08:47:23Z

@AliSoftware @djbe any decision or change here please?
It has been 2.5y and I believe it is still a valid problem and solution for some projects with mixed Obj-C / Swift codebase.

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.

Abstract45 · 2023-01-14T00:21:12Z

@AliSoftware @djbe any decision or change here please? It has been 2.5y and I believe it is still a valid problem and solution for some projects with mixed Obj-C / Swift codebase.

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?

lukaskukacka · 2023-01-14T10:11:13Z

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 😀
But I'd like a confirmation whether or not this will be accepted and merged if I make the PR up-to-date again. Just to avoid wasting my efforts like with the original solution in 2018 (!!!)

lukaskukacka · 2023-10-30T11:10:48Z

Hello @djbe @AliSoftware,
could you guys please give some opinions on future of this PR?

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
Lukas

AliSoftware reviewed Sep 7, 2018

View reviewed changes

lukaskukacka commented Sep 12, 2018

View reviewed changes

djbe added this to the SwiftGen 6.1 milestone Sep 26, 2018

lukaskukacka added 2 commits September 28, 2018 19:47

SwiftGen#489 - Added option to generate Objective-C compatible String…

68cb890

…s Swift code

SwiftGen#489 - Fixed Swiftlint false-positive for Obj-C compatible St…

cc6f4ad

…rings

lukaskukacka force-pushed the 489-ObjCCompatibleStrings branch from 501d6ec to cc6f4ad Compare September 28, 2018 17:51

Merge branch 'master' into 489-ObjCCompatibleStrings

3ace43a

djbe modified the milestones: 6.1.0, 6.2.0 Jan 26, 2019

djbe mentioned this pull request Mar 3, 2019

Strings: Obj-C templates #378

Merged

lukaskukacka mentioned this pull request Mar 7, 2019

Strings: Add support to generate Objective-C compatible Strings Swift code #489

Open

AliSoftware changed the base branch from master to stable June 10, 2020 23:19

djbe modified the milestones: 6.3.0, Nice to Have Jun 18, 2020

djbe changed the title ~~#489 - Added option to generate Objective-C compatible Strings Swift code~~ Strings: option to generate Objective-C compatible Swift code Oct 4, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Strings: option to generate Objective-C compatible Swift code #490

Strings: option to generate Objective-C compatible Swift code #490

lukaskukacka commented Sep 7, 2018 •

edited by djbe

AliSoftware commented Sep 7, 2018

AliSoftware left a comment

AliSoftware Sep 7, 2018 •

edited

lukaskukacka Sep 7, 2018 •

edited

AliSoftware Sep 7, 2018 •

edited

lukaskukacka Sep 7, 2018

AliSoftware commented Sep 7, 2018 •

edited

lukaskukacka commented Sep 7, 2018

AliSoftware commented Sep 7, 2018

lukaskukacka Sep 12, 2018

lukaskukacka commented Sep 28, 2018

AliSoftware commented Sep 28, 2018

lukaskukacka commented Sep 28, 2018

djbe commented Sep 28, 2018

AliSoftware commented Sep 28, 2018 •

edited

ismetanin commented Jan 15, 2020

lukaskukacka commented Jan 15, 2020

AliSoftware commented May 17, 2020

lukaskukacka commented Dec 6, 2022

Abstract45 commented Jan 14, 2023

lukaskukacka commented Jan 14, 2023

lukaskukacka commented Oct 30, 2023

Strings: option to generate Objective-C compatible Swift code #490

Are you sure you want to change the base?

Strings: option to generate Objective-C compatible Swift code #490

Conversation

lukaskukacka commented Sep 7, 2018 • edited by djbe

Technical solution

Decisions

Swift 4

Flat only

AliSoftware commented Sep 7, 2018

AliSoftware left a comment

Choose a reason for hiding this comment

AliSoftware Sep 7, 2018 • edited

Choose a reason for hiding this comment

lukaskukacka Sep 7, 2018 • edited

Choose a reason for hiding this comment

AliSoftware Sep 7, 2018 • edited

Choose a reason for hiding this comment

lukaskukacka Sep 7, 2018

Choose a reason for hiding this comment

AliSoftware commented Sep 7, 2018 • edited

lukaskukacka commented Sep 7, 2018

AliSoftware commented Sep 7, 2018

lukaskukacka Sep 12, 2018

Choose a reason for hiding this comment

lukaskukacka commented Sep 28, 2018

AliSoftware commented Sep 28, 2018

lukaskukacka commented Sep 28, 2018

djbe commented Sep 28, 2018

AliSoftware commented Sep 28, 2018 • edited

ismetanin commented Jan 15, 2020

lukaskukacka commented Jan 15, 2020

AliSoftware commented May 17, 2020

lukaskukacka commented Dec 6, 2022

Abstract45 commented Jan 14, 2023

lukaskukacka commented Jan 14, 2023

lukaskukacka commented Oct 30, 2023

lukaskukacka commented Sep 7, 2018 •

edited by djbe

AliSoftware Sep 7, 2018 •

edited

lukaskukacka Sep 7, 2018 •

edited

AliSoftware Sep 7, 2018 •

edited

AliSoftware commented Sep 7, 2018 •

edited

AliSoftware commented Sep 28, 2018 •

edited