Update project to Swift 3 by ahtierney · Pull Request #201 · SwiftGen/SwiftGen

ahtierney · 2016-11-16T17:08:07Z

run migrator to swift 3
update pods to latest, swift 3 compatible versions
audit APIs for swift 3 compliance

Here's a migration to swift 3 as per issue #169 with a goal to bring all the project dependencies up to date.

AliSoftware · 2016-11-16T17:19:19Z

Cool, thanks for this work on the migration!

I think you should add an item to your checklist: "refactor the API to be compliant with Swift 3 naming guidelines". It's cool if it compiles, but this migration is the best occasion to also make the internal method names Swift3-compliant too 😉

As per switching to Swift3 being the default template, that would mean a major version bump of SwiftGen IMHO, so that people using the current version will still have their generated code compatible even if they kept the default template.

If we manage to merge this PR before the imminent SwiftGen 4.0.0, it might be the right time. But it might be better to do separate PRs (this one to convert these internal code of SwiftGen too Swift 3 without any visible external changes for the users, and a separate one to rename there templates to make the Swift 3 templates the default and update the documentation accordingly) this way we can merge this one quickly then decide about the other, public migration of the templates if we have time to integrate in 4.0 or will do in a 5.0. I'm more in favor of the last solution to allow more flexibility at least it will allow us to decide art the time we decide to release there next version!

ahtierney · 2016-11-16T17:30:24Z

Sounds like a plan! I'll do an API audit and aim to keep this branch internal / non-breaking. Thanks!

ahtierney · 2016-11-16T21:38:47Z

I've done the API audit and merged up with master. Naming is hard, so let me know if there's anything you take issue with!

djbe · 2016-11-16T22:11:41Z

GenumKit/Parsers/AssetsCatalogParser.swift

  static let imageSetExtension = "imageset"

-  fileprivate func process(_ items: [[String: AnyObject]], prefix: String = "") {
+  fileprivate func processCatalogItems(_ items: [[String: AnyObject]], prefixedWith prefix: String = "") {


Maybe process(items: , withPrefix: )? Or processCatalog(items: , withPrefix: )

djbe · 2016-11-16T22:13:30Z

GenumKit/Parsers/StoryboardParser.swift

  }

-  public func addStoryboardAtPath(_ path: String) {
+  public func addStoryboard(atPath path: String) {


To be consistent with your naming in fonts & color parsers, I'd change this to addStoryboard(at path: )

👍 good catch

djbe · 2016-11-16T22:14:13Z

GenumKit/Parsers/StringsFileParser.swift


-    public static func fromFormatString(_ format: String) -> [PlaceholderType] {
-      return StringsFileParser.typesFromFormatString(format)
+    public static func placeholders(fromFormatString str: String) -> [PlaceholderType] {


Leave the parameter type out: placeholders(fromFormat str: String)

djbe · 2016-11-16T22:15:01Z

GenumKit/Parsers/StringsFileParser.swift


  // "I give %d apples to %@" --> [.Int, .String]
-  fileprivate static func typesFromFormatString(_ formatString: String) -> [PlaceholderType] {
+  fileprivate static func placeholders(fromFormatString formatString: String) -> [PlaceholderType] {


Same as above: placeholders(fromFormat formatString: String)

ahtierney · 2016-11-17T18:53:13Z

It looks like CI is having an issue with it's pods repo. Based on what I'm seeing it seems like I need to add a pod repo update to the .travis.yml file's install step. Any thoughts regarding this? Apart from CI I think I've got everything in now.

djbe · 2016-11-17T18:56:56Z

Problem with that is that it takes a really long time. See here:
travis-ci/travis-ci#6473

AliSoftware · 2016-11-17T18:57:38Z

Indeed.
pod repo update isn't an installation step per se, so I'd instead add it as a step before the linting, or even in the same command: pod repo update && pod lib lint …

djbe · 2016-11-17T18:58:37Z

In the thread I linked, they mention using a new osx image, which version do we use?

AliSoftware · 2016-11-17T18:58:41Z

Another solution to avoid it taking too long is to use the --quick flag on pod lib lint. Would probably be better.

ahtierney · 2016-11-17T18:59:31Z

yea if we can avoid updating that pods that would be best, does --quick skip repo validation?

djbe · 2016-11-17T18:59:33Z

try changing the osx image to:
osx_image: xcode8.1
and see if it fixes the issue

AliSoftware · 2016-11-17T19:01:26Z

--quick avoids any linting that would need to download anything, so yeah.

I think maybe we should do both: upgrade to the Travis image xcode8.1 and use --quick, right?

ahtierney · 2016-11-17T19:02:00Z

Sounds good to me

ahtierney · 2016-11-17T20:44:32Z

success 🎉

AliSoftware · 2016-11-17T20:48:57Z

Good job! 👍

Haven't much time to audit the new API, maybe if someone else can audit before merging (as it's the kind of migration where it's easy to miss something)?

ahtierney · 2016-11-17T20:54:54Z

Definitely in the more eyes the better camp. I'll see if I can recruit @dostrander to it.

djbe · 2016-11-17T20:57:32Z

I'll see if I can get this to work, could be useful:
https://github.com/dduan/needless

djbe · 2016-11-17T21:31:55Z

All right, either needless doesn't find much, or we've got most of it covered. The only thing it found was in SwiftIdentifier.swift:

func swiftIdentifier(fromString string: String

should become

func swiftIdentifier(from string: String

I tested GenumKit, swiftgen-cli, the unit tests and the expected output.

ahtierney · 2016-11-17T21:41:34Z

I'm down for that. I'll push the change in a minute.

AliSoftware

Various comments to be addressed.

Especially take a closer look at the duplicate build phases that seems to have been added, and at if fileprivate could be reverted to private in most cases.

AliSoftware · 2016-11-20T16:30:18Z

GenumKit/Parsers/AssetsCatalogParser.swift


-  public func addImageName(name: String) -> Bool {
+  @discardableResult
+  public func addImageName(_ name: String) -> Bool {


add(imageName: String) -> Bool would be more Swift-3-y maybe?

addImage(named name: String) to match what you used in the color parser addColor(named name: String)

I went back and fourth on this one, I think because of the collection is actually imageNames I have to confess I'm still a little unclear on if there is an objective differentiator between addImage() add(image:) 😬 I agree though, addImage(named name: String) is best for api continuity.

AliSoftware · 2016-11-20T16:32:15Z

GenumKit/Parsers/ColorsFileParser.swift


-private func parseHexString(hexString: String) throws -> UInt32 {
-  let scanner = NSScanner(string: hexString)
+fileprivate func parseHex(_ hexString: String) throws -> UInt32 {


parse(hex:)?

AliSoftware · 2016-11-20T16:54:28Z

GenumKit/Parsers/ColorsFileParser.swift


 public final class ColorsTextFileParser: ColorsFileParser {
-  public private(set) var colors = [String:UInt32]()
+  public fileprivate(set) var colors = [String:UInt32]()


Is this change (private -> fileprivate) really necessary?
That's probably the migrator doing this change for us but Swift 3's private might be good enough in lots of cases I think, worth reviewing them all to check if they can't be reverted to private.

agree, I'll do a full audit. A lot of these I think are code style between extension grouping.

AliSoftware · 2016-11-20T16:55:32Z

GenumKit/Parsers/ColorsFileParser.swift

-    let content = try NSString(contentsOfFile: path, encoding: NSUTF8StringEncoding)
-    let lines = content.componentsSeparatedByCharactersInSet(NSCharacterSet.newlineCharacterSet())
-    let whitespace = NSCharacterSet.whitespaceCharacterSet()
+    let content = try NSString(contentsOfFile: path, encoding: String.Encoding.utf8.rawValue)


I wonder why I didn't use methods on String rather than NSString here, or use PathKit's methods to get the content of a file rather than depend on Foundation/NSString…

AliSoftware · 2016-11-20T16:59:49Z

GenumKit/Stencil/Contexts.swift

+  case let (l?, r?):
+    return l < r
+  case (nil, _?):
+    return true


I'm not a fan of reintroducing comparison on optionals (I quite agree with the rationale behind SE-121 tbh) — especially the fact that nil < nil is true is quite disturbing.

I think we can use a better way to do that, like check where that comparison is used and do exploit checks with guard let et al. instead.

Agree. I'm kind of impressed the converter produces this 🤔

AliSoftware · 2016-11-20T17:03:31Z

GenumKit/Stencil/GenumTemplate.swift

    let templateStringWithMarkedNewlines = templateString
-      .stringByReplacingOccurrencesOfString("\n\n", withString: "\n\u{000b}\n")
-      .stringByReplacingOccurrencesOfString("\n\n", withString: "\n\u{000b}\n")
+        .replacingOccurrences(of: "\n\n", with: "\n\u{000b}\n")


Not sure why I did the replacement twice, but I think that was intentional (like for cases like \n\n\n, the first replacement only replaces the first 2 \n and the second takes care of the remaining ones)… so we should keep that dual replacement in place even after the Swift 3 migration.

If so, then even a double replacement won't cover all cases. It might be better to use a regex for this:

templateString.replacingOccurrences(of: "\n\n+", with: "\n\u{000b}\n", options: .regularExpression)

AliSoftware · 2016-11-20T17:05:52Z

SwiftGen.playground/Pages/Fonts-Demo.xcplaygroundpage/Contents.swift

+let helveticaNeue = FontFamily.HelveticaNeue.Regular.font(size: 20.0)

-let helveticaBoldBig = FontFamily.Helvetica.Bold.font(100.0)
+let helveticaBoldBig = FontFamily.Helvetica.Bold.font(size: 100.0)


Don't we generate lowercase cases now, like Helvetica.bold and not Helvetica.Bold? I mean, if the Playground is in Swift 3 (which this font(size: 100) instead of font(100) seems to suggest, should we assume that the code in the Playground might reflect the code from the -t swift3 template, or should we still use the code generated by the default template in the Swift 3 playground?

I was a little unclear, but I think it should be swift 3 examples. I'll fix this.

AliSoftware · 2016-11-20T17:08:33Z

SwiftGen.xcodeproj/project.pbxproj

 			buildConfigurationList = 09A87B451BCC9B8000D9B9F5 /* Build configuration list for PBXNativeTarget "swiftgen" */;
 			buildPhases = (
+				54570B6ACBB962B5ED01A405 /* [CP] Check Pods Manifest.lock */,
 				BEC9F3A158B764C68AF03EF9 /* [CP] Check Pods Manifest.lock */,


Seems strange that the Build Phase got duplicated here. We gotta double-check that.

AliSoftware · 2016-11-20T17:10:02Z

SwiftGen.xcodeproj/project.pbxproj

 				C40C2D80E9C1FEEAA4FD6B14 /* [CP] Embed Pods Frameworks */,
 				6C7EBDA1392A1BFCC9F0A2DA /* [CP] Copy Pods Resources */,
+				1CAEE405320C3DBD251A68B3 /* 📦 Embed Pods Frameworks */,
+				B8FA4CC6E874346645F002E3 /* 📦 Copy Pods Resources */,


Same, seems like you used an outdated CocoaPods version to regenerate that project, the version of CP when I (it happens that it was me who did that on CP 😄) introduced the emoji in the build phases names, and which got reverted to [CP] in a later version… Gotta clean that up and regenerate the project with a recent version of CP probably…

Do you think it would it be beneficial to put a Gemfile in the project to make sure people are on the same version of cocoapods?

Probably a good idea indeed 👍

Dunno about you, but my cocoapods is installed system wide, and I don't think a gemfile will affect that (never used them).

@djbe mine are also installed system-wide, that doesn't impact that 😉

When you run bundle install bundler will install all the required gems, in the right versions, system-wide (unless told otherwise). Then you just need to run bundle exec pod install (instead of just pod install) to be sure to run the right version of your system-wide-installed CocoaPods, even if that one (the one defined in the Gemfile) isn't there last version installed in your system.

Good catch 😬 I'll sort this out. I can add bundler too, maybe once #169 is all sorted out.

AliSoftware · 2016-11-20T17:11:10Z

SwiftGen.xcodeproj/project.pbxproj

 			shellScript = "\"${SRCROOT}/Pods/Target Support Files/Pods-swiftgen/Pods-swiftgen-resources.sh\"\n";
 			showEnvVarsInLog = 0;
 		};
+		F815D65995A8B21A935964D2 /* 📦 Copy Pods Resources */ = {


Yup, it's clear that this build phase is the duplicate of the one just before…

ahtierney · 2016-11-21T01:14:29Z

Thanks for the feedback!

- fixed strings test

- misc style tweaks - changelog tweak

ahtierney · 2016-11-21T19:14:02Z

Feedback is up:

Rebase off master
Naming changes
converted all fileprivate to private where possible (left one case in the asset parser where extensions could be restructured, but didn't want to be too opinionated about it)
re-gen'd playgrounds
cleaned up pods but running deintegrate and rebuilding

djbe

Just a small change/improvement

djbe · 2016-11-22T18:10:56Z

GenumKit/Stencil/GenumTemplate.swift

    let templateStringWithMarkedNewlines = templateString
-      .stringByReplacingOccurrencesOfString("\n\n", withString: "\n\u{000b}\n")
-      .stringByReplacingOccurrencesOfString("\n\n", withString: "\n\u{000b}\n")
+        .replacingOccurrences(of: "\n\n", with: "\n\u{000b}\n")


If so, then even a double replacement won't cover all cases. It might be better to use a regex for this:

templateString.replacingOccurrences(of: "\n\n+", with: "\n\u{000b}\n", options: .regularExpression)

ahtierney · 2016-11-22T19:02:28Z

@djbe I think I agree in principle, but if I understand the code correctly the intent is not to flatten inbound new lines, but to preserve them.
So "\n\n\n\n\n\n foobaz \n\n\n\n\n\n\n" should output the same, not \n\n foobaz \n\n
Correct me if I'm wrong here. Either way might make sense to have some tests for this so expected behavior is clear and it always behaves the same.

djbe · 2016-11-22T20:47:11Z

Huh, yeah. I honestly just took a look at the issue (stencilproject/Stencil#22) and there they just mention extra empty lines they want to get rid of.

Not needed

djbe · 2016-11-25T21:54:07Z

@AliSoftware could we get this merged? I think al your feedback was covered.

AliSoftware

Ok just some nitpickings before we go, nothing much.

Sorry for being so pedantic, but this migration is the best time to be sure everything is clean 😉

AliSoftware · 2016-11-25T22:04:31Z

CHANGELOG.md

  ensuring that the parser correctly handles namespaced folders.  
  [David Jennes](https://github.com/djbe)
  [#199](https://github.com/AliSoftware/SwiftGen/pull/199)
+* Swift 3 migration


Missing the full-stop + 2-spaces at the end of this line so that the rendered markdown is formatted properly.

AliSoftware · 2016-11-25T22:06:16Z

GenumKit/Stencil/Contexts.swift

-    let strings = entries.sort { $0.key < $1.key }.map { entryToStringMapper($0, []) }
-    let structuredStrings = structure(entries, mapper: entryToStringMapper, currentLevel: 0, maxLevel: 5)
+    let strings = entries
+        .sorted { $0.key.lowercased() < $1.key.lowercased() }


Might be better to use str1.caseInsensitiveCompare(str2) or similar here, to ensure that any string case but also any diacritics and other niceties of Unicode are insensitively-compared properly (even if those are very rare to uncommon in keys)

AliSoftware · 2016-11-25T22:06:49Z

GenumKit/Parsers/AssetsCatalogParser.swift


 extension AssetsCatalogParser {
-  private func loadAssetCatalogContents(path: String) -> [[String: AnyObject]]? {
+  fileprivate func loadAssetCatalog(at path: String) -> [[String : AnyObject]]? {


SwiftLint rules usually prefer no space before : (but one space after) : [[String: AnyObject]]? was fine ^^

AliSoftware · 2016-11-25T22:07:48Z

GenumKit/Parsers/AssetsCatalogParser.swift


-  public func addImageName(name: String) -> Bool {
+  @discardableResult
+    public func addImage(named name: String) -> Bool {


Huh, why the indentation change here? Seems like a misindentation was introduced by mistake

AliSoftware · 2016-11-25T22:09:35Z

GenumKit/Parsers/ColorsFileParser.swift


 public final class ColorsTextFileParser: ColorsFileParser {
-  public private(set) var colors = [String:UInt32]()
+  public private(set) var colors = [String : UInt32]()


Ditto here, and in some other places as well, just quick find/replace all.
(note that some of those might be from me and my old code-style, not all from changes in this PR? But would be nice to uniform this to match SwiftLint's rules anyway)

djbe · 2016-11-25T22:18:13Z

wooo, that works!
I've made the changes + searched globally for :

AliSoftware · 2016-11-25T22:53:03Z

Hell yeah! Let's merge! 🎉

djbe · 2016-11-25T22:53:42Z

Wooo! 😄

ahtierney · 2016-11-25T23:27:53Z

Awesome! Thanks a lot guys. I'll rebase and tune up #204 this weekend!

AliSoftware · 2016-11-25T23:30:35Z

@ahtierney Cool!
Just invited you as an official Collaborator on the project, in case you prefer to create branches directly on the main repo (instead of in your fork) and some other niceties 😉

ahtierney · 2016-11-26T18:49:58Z

@AliSoftware Awesome, thank you!

@dostrander

As suggested by @dostrander on #201 (comment)

AliSoftware · 2016-12-04T00:56:32Z

@dostrander @djbe I've added a Gemfile in 152d48e to be able pin the version of CocoaPods used by SwiftGen (while still allowing you to have whatever version you prefer system-wide)

For those of you not familiar with the usage of a Gemfile to pin gems like CocoaPods to a specific version, you can read more on the topic here, but basically, to use it:

Initial setup

If you haven't the bundler gem installed, run gem install bundler. This has to be done only once, for the very first time.
Run bundle install once, to install (system-wide) all the gems (with their corresponding version) listed in the Gemfilein case you don't have them already.
- For example as for now, because the Gemfile contains gem 'cocoapods', '1.2.0.beta.1' this will install exactly this version of CocoaPods if you don't have it already

Everyday usage

Every time you want to execute a CocoaPods command on the SwiftGen project, like pod install for example, use bundle exec pod install instead, to tell it to use the pod binary specified in the Gemfile, and not any other default one used by the system

For example on my Mac I currently have both CocoaPods 1.1.0 and 1.2.0.beta.1. If (anywhere, system-wide) I do pod --version this will default to 1.1.0 (because stable versions have priority in RubyGems). But if I'm in the SwiftGen's directory and I do bundle exec pod --version this will use 1.2.0.beta.1 instead, even if 1.1.0 is the default system-wide and even if I have a 1.3.0 installed on my system in the future.

This ensures consistency in the version every contributor uses when working as a team on SwiftGen together.

ahtierney force-pushed the feature/ahtierney/swift3 branch 2 times, most recently from cccf0df to b7a12f9 Compare November 16, 2016 21:35

djbe reviewed Nov 16, 2016

View reviewed changes

ahtierney force-pushed the feature/ahtierney/swift3 branch from 3e81735 to 68775ad Compare November 17, 2016 00:16

ahtierney mentioned this pull request Nov 19, 2016

Issue #169 — Reorganize project as an app bundle #204

Merged

AliSoftware mentioned this pull request Nov 20, 2016

Convert project to Swift 3 and app bundle #169

Closed

2 tasks

AliSoftware requested changes Nov 20, 2016

View reviewed changes

AliSoftware mentioned this pull request Nov 20, 2016

Cut a release to 3.0.2 #196

Closed

djbe added this to the Next patch (4.0.1) milestone Nov 20, 2016

- change swiftIdentifier method signature

cd0f99c

ahtierney added 3 commits November 20, 2016 19:57

- First pass feedback

5746924

- fixed strings for strict alphabetical ordering

d774a7c

- fixed strings test

- Fileprivate audit

8cabdf1

ahtierney force-pushed the feature/ahtierney/swift3 branch from c07261a to 8cabdf1 Compare November 21, 2016 04:47

ahtierney added 2 commits November 21, 2016 10:32

- Playground tweaks

190b373

- misc style tweaks - changelog tweak

- cleaned up duped pod build phases

c11c336

ahtierney force-pushed the feature/ahtierney/swift3 branch from 454a4c2 to c11c336 Compare November 21, 2016 18:34

djbe mentioned this pull request Nov 21, 2016

Multiple asset catalogs - no image found #207

Closed

djbe previously requested changes Nov 22, 2016

View reviewed changes

AliSoftware requested changes Nov 25, 2016

View reviewed changes

djbe added 2 commits November 25, 2016 23:15

whitespace

928473f

key sorting

9a70cd2

AliSoftware approved these changes Nov 25, 2016

View reviewed changes

AliSoftware merged commit db8d8b0 into SwiftGen:master Nov 25, 2016

AliSoftware added a commit that referenced this pull request Dec 4, 2016

Adding a Gemfile

152d48e

As suggested by @dostrander on #201 (comment)

ffittschen mentioned this pull request Jul 29, 2024

Sorting of entries in StencilContext not deterministic #1135

Open

Conversation

ahtierney commented Nov 16, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Nov 16, 2016

Uh oh!

ahtierney commented Nov 16, 2016

Uh oh!

ahtierney commented Nov 16, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

djbe commented Nov 17, 2016

Uh oh!

AliSoftware commented Nov 17, 2016

Uh oh!

djbe commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Nov 17, 2016

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

djbe commented Nov 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware commented Nov 17, 2016

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

AliSoftware commented Nov 17, 2016

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

djbe commented Nov 17, 2016

Uh oh!

djbe commented Nov 17, 2016

Uh oh!

ahtierney commented Nov 17, 2016

Uh oh!

AliSoftware left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ahtierney commented Nov 16, 2016 •

edited

Loading

djbe commented Nov 17, 2016 •

edited

Loading

djbe commented Nov 17, 2016 •

edited

Loading