Images: dot syntax #206

djbe · 2016-11-21T01:06:09Z

Follow up of #199, implements the dot-syntax for asset catalogs (mentioned in #52).

For now only has the swift3 template, I want some input before I do the swift 2 version.

* master: key sorting whitespace - cleaned up duped pod build phases - Playground tweaks - misc style tweaks - changelog tweak - Fileprivate audit - fixed strings for strict alphabetical ordering - fixed strings test - First pass feedback - change swiftIdentifier method signature - Bump xcode version and doing quick lint to avoid travis pod repo issue : travis-ci/travis-ci#6473 - Fixed genumkit swift version - Playgrounds to swift 3 - Naming tweaks from feedback - Audit internal APIs for swift 3 compliance - Bump swift version and change log - Bump swift version and change log - Fixed font tests and made font parsing alphabetical for predictablity - fixed swift 3 index regression - fixed compiler errors - All tests passing, except font ordering issue - Swift 3 syntax to building - update pods to swit 3/latest - Ran Migrator to swift 3 # Conflicts: # GenumKit/Parsers/AssetsCatalogParser.swift # GenumKit/Stencil/Contexts.swift # SwiftGen.xcodeproj/project.pbxproj

AliSoftware · 2016-11-26T16:53:23Z

GenumKit/Parsers/AssetsCatalogParser.swift

+  struct Entry {
+    var name: String
+    var value: String
+    var items: [Entry]?


If an Entry is either an image (with name and value) or a namespace (with name and sub-entries), shouldn't we use an enum with associated values for that instead?

Like either:

enum Entry { case image(name: String, value: String) indirect case namespace(name: String, entries: [Entry]) }

Or

struct Entry { var name: String enum Content { case image(value: String) case namespace(entries: [Entry]) } }

Haven't used enums with associated values yet (although I've read about them). I'll look into it.

I think the first solution (a simple enum with associated values) would be easier to use than a struct with enum inside.

Using a struct with inner enum is only useful if all cases of the enum share multiple properties, like enough to want to avoid repeating each property in each enum case's associated values.
But here we only have one common property so I think it's not worth it and integrating name directly in each enum case would be easier to manipulate.

And that way you won't need dedicate inits, you can just create Entry.image(name: "foo", value: "parent/foo") and Entry.namespace(name: "parent", entries: children).

PS: Note that I'm not sure if the indirect keyword is really needed. I think so, because the case references the enum being defined, but it's worth trying without to be sure we can't avoid it.

AliSoftware · 2016-11-26T16:55:33Z

GenumKit/Parsers/AssetsCatalogParser.swift

  }
 }

 // MARK: - ACTool

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


It would be nice to add a doc-comment to this function to document the typical structure of the PLIST/JSON returned by actool, like the keys returned, how they look for image entries, for namespaced images, for namespaces themselves… Would help understand the rest of the code parsing it (especially parse(items😃)

(I know, I should have mentioned that in the other PR about actool migration ^^)

AliSoftware · 2016-11-26T16:59:01Z

GenumKit/Stencil/Contexts.swift

+    )
+  }
+
+  func justValues(entries: [Entry]) -> [String] {


Should probably be private (or fileprivate)?

AliSoftware · 2016-11-26T16:59:06Z

GenumKit/Stencil/Contexts.swift

+    return result
+  }
+
+  func structure(entries: [Entry], currentLevel: Int = 0, maxLevel: Int = 5) -> [[String: Any]] {


Should probably be private (or fileprivate)?

AliSoftware · 2016-11-26T16:59:13Z

GenumKit/Stencil/Contexts.swift

+    }
+  }
+
+  func flatten(entries: [Entry]) -> [[String: Any]] {


Should probably be private (or fileprivate)?

AliSoftware · 2016-11-26T17:00:14Z

UnitTests/TestSuites/ImagesTests.swift

-  }
+	}
+
+	func testFileWithDotSyntax() {


Seems like you don't have the same indentation settings as the ones defined in the project (2-spaces indentation)?

I don't know, sometimes Xcode forgets them, or something else.

Personally I always use tabs so that's why it sometimes gets through, even though I try to be careful.

AliSoftware · 2016-11-26T17:03:02Z

GenumKit/Parsers/AssetsCatalogParser.swift


        if let providesNamespace = item[AssetCatalog.providesNamespace.rawValue] as? NSNumber,
-            providesNamespace.boolValue {
-          processCatalog(items: children, withPrefix: "\(prefix)\(filename)/")
+          providesNamespace.boolValue {


Doesn't Swift automatically bridge boolean NSNumber to Bool with as?? Especially since the AnyObject -> Any proposal got in?

I mean, we could probably do if let providesNamespace = item[AsstCatalog.providesNamespace.rawValue] as? Bool directly maybe? (At least it's worth a try)

I think this was written before the swift3 merge. Don't know, but you're right, it should be bridged.

AliSoftware · 2016-11-26T17:06:36Z

GenumKit/Parsers/AssetsCatalogParser.swift

-  fileprivate func processCatalog(items: [[String: AnyObject]], withPrefix prefix: String = "") {
+  fileprivate func process(items: [[String: Any]], withPrefix prefix: String = "") -> [Entry] {
+    var result = [Entry]()
+
    for item in items {
      guard let filename = item[AssetCatalog.filename.rawValue] as? String else { continue }


I totally forgot to see that in the other PR about actool migration, but if you're only using the enum AssetCatalog as a way to group constants together (and never use the cases other than via their rawValue to get the String constant), I'd suggest you to use an enum with no cases, but with static let constants namespaced in the enum instead:

private enum AssetCatalogKeys { static let children = "children" static let filename = "filename" static let providesNamespace = "provides-namespace" static let root = "com.apple.actool.catalog-contents" }

So we'd be using the enum just as some sort of way to namespace constants?

Yup. That's how I namespace all my constants in my projects now, and have been loving it!

Like I very often have stuff like this in my projects:

struct SomeModel { enum JSONKeys { static let firstname = "firstName" static let lastname = "lastName" static let dob = "dateOfBirth" } let firstname: String let lastname: String let dateOfBirth: String init?(json: [String: Any]) { guard let fn = json[JSONKeys.firstname] as? String, let ln = json[JSONKeys.lastname] as? String, let dob = json[JSONKeys.dob] as? String else { return nil } self.firstname = fn self.lastname = ln self.dateOfBirth = dob } }

AliSoftware

Don't hesitate to rebase on top of master, instead of merging master into your branch.

Both have pros and cons, but merging master into your branch means that you can get quite a 🍝 git tree sometimes. And it's then harder to rebase stuff like a PR that has been opened on top of another (like 204 is on top of 201) and make the latest PR only contain the diff commits (instead it adds more 🍝 )

Sure, rebasing instead of merging also have cons, like rewriting history meaning that other people who already have pulled your commit won't have valid commits anymore, but on a feature branch used for a PR, and on which typically only one people commits onto, it seems less a problem than the git tree 🍝 brought by merging master. So sure it's a matter of preferences but I prefer clean trees personally 😉 🌲

AliSoftware · 2016-11-26T17:30:22Z

GenumKit/Parsers/AssetsCatalogParser.swift

-  fileprivate func loadAssetCatalog(at path: String) -> [[String: Any]]? {
-    let command = Command("xcrun", arguments: "actool", "--print-contents", path)
+  fileprivate func loadAssetCatalog(at path: Path) -> [[String: Any]]? {
+    let command = Command("xcrun", arguments: "actool", "--print-contents", String(describing: path))


Doesn't PathKit's Path have a dedicated property to get the path string?
I mean, String(describing:) feels more intended for debug representation purposes to me, while something like path.path or path.string if it exists would feel more right (a bit like you'd use url.absoluteString)

You'd think that, wouldn't you? There isn't any public one, only internal methods unfortunately.

It does conform to CustomStringConvertible, but that's what I'm using.

Not even path.description from some CustomStringConvertible conformance?

Yeah, but what's better? String(describing: path) or path.description?

I'll create a PR in PathKit for something better, but I'll leave it up to you what we should use for now.

I think path.description feels more like a property on Path while String(describing: path), although doing the same thing, feels more like a conversion or builder to me.

djbe · 2016-11-26T18:08:44Z

What we can do (and kylef from Commander does this), is when merging the pull request to do "squash and merge", that way you'll always have a clean tree.

AliSoftware · 2016-11-26T18:10:06Z

👍

AliSoftware

Fix indentation
Add CHANGELOG entry

AliSoftware · 2016-11-29T14:02:59Z

GenumKit/Parsers/AssetsCatalogParser.swift

    }
  }

+  guard !found else { return false }
+  entries.append(Entry.image(name: name, value: name))


indentation (from lines 17 to 26) seems wrong 😉

AliSoftware · 2016-11-29T14:05:33Z

GenumKit/Parsers/AssetsCatalogParser.swift

+     <key>com.apple.actool.catalog-contents</key>
+     <array>
+       <dict>
+			 <key>children</key>


⚠️ tabs indentation

AliSoftware · 2016-11-29T14:10:06Z

UnitTests/expected/Images-File-Dot-Syntax.swift.out

+    static let Orange: AssetType = "Round/Orange"
+
+    struct Red {
+      static let Apple: AssetType = "Round/Apple"


Why isn't the value of this generated constant "Round/Red/Apple" instead of just "Round/Apple"?
Is it because the group in the asset catalogs doesn't have provides-namespace checked, so that's normal and intentional?
In that case I'm wondering if we should create the intermediate structure struct Red if it doesn't provide namespace. I'm thinking YES (and so in this case this output is 👍 ), as otherwise the developer wouldn't have created a group in its asset catalog… but just wanted to start the discussion in case anyone had a different opinion.

This is intentional. Only namespaced folders modify the value (prefixing them with the folder's name).

I also think yes, because the structure in the catalog is there, and SwiftGen should provide an added value by offering this deeper structure. We could pass this extra structure information on to the template, although I'll have to think about how to best implement that.

djbe · 2016-11-29T18:03:54Z

BTW, I based the templates on the strings dot syntax. Any reason why we use nested structs instead of nested enums for namespacing?

AliSoftware · 2016-11-29T18:50:23Z

I think it's just because I overlooked the PR implementing those templates at that time, would indeed seem better to use enums instead of structs here.

djbe · 2016-11-29T18:53:31Z

Did this maybe depend on swift 3?

AliSoftware · 2016-11-29T18:57:18Z

Nah I don't think it did. enums as namespaces was already possible in Swift 2…

AliSoftware · 2016-12-01T12:52:26Z

Conflict to fix so we can merge 😉

djbe · 2016-12-01T12:53:07Z

Oh you want to do a minor release instead of patch? Sure, gimme a minute.

AliSoftware · 2016-12-01T12:54:52Z

Ah, didn't realize that this bumped to the minor version… why is that btw?

djbe · 2016-12-01T12:55:39Z

New feature? (new template)

AliSoftware · 2016-12-01T12:55:56Z

I mean, before this PR, people using namespaces didn't have the dot namespace in the generated code, now they will… that means breaking, right?

djbe · 2016-12-01T12:56:24Z

No the old workflow still works, it still generates a flat enum with all cases.

AliSoftware · 2016-12-01T12:57:35Z

Oh, sorry, you're right, I thought we already had a (non-working) dot-syntax template for Images, (I got confused with the one we already have for Strings). If that's a new template then yeah, minor it is 😉

In that case yeah, maybe we should do a patch version real quick first, before merging this and starting a new minor, right.

djbe · 2016-12-01T12:58:51Z

Yeah, I mean, I still have to check this works from command line 😅
-t somethingsomething to switch templates right?

AliSoftware · 2016-12-01T12:59:07Z

yup!

djbe · 2016-12-01T13:01:01Z

So, I think I'll wait until 4.0.1 is released before I fix the conflict in this, otherwise I'll keep having to merge changes.

AliSoftware · 2016-12-01T13:09:04Z

Yup, probably a good idea indeed 😉

Can't wait for #204 to be able to test those command line invocations right from Xcode instead of having to launch the Terminal and rake install for that 😉

(Will review #204 rn btw, it's about time we move forward on this, would help a lot the dev process)

djbe · 2016-12-01T13:09:50Z

Definitely! 🎉

djbe added 4 commits November 20, 2016 23:24

parse asset catalog into simple structure

1048f06

images: dot syntax swift3 template

74bac88

we still need the “paths” of an asset

a931933

unit test

40470f9

djbe added this to the Next minor (4.1.0) milestone Nov 21, 2016

djbe mentioned this pull request Nov 21, 2016

Multiple asset catalogs - no image found #207

Closed

djbe added 4 commits November 21, 2016 21:05

swift 2.3 stencil and test

0a878fc

moar pathkit

960623f

Revert some of the last commit, as it’s moved to another PR

cb27300

AliSoftware requested changes Nov 26, 2016

View reviewed changes

djbe added 3 commits November 26, 2016 18:17

Just use enum to namespace constants

d68fbb7

whitespace

28d3ca2

Merge branch 'master' into feature/structured-images

5078d03

AliSoftware requested changes Nov 26, 2016

View reviewed changes

djbe added 3 commits November 26, 2016 19:16

use enum with associated values

0245083

documentation for catalog parser

09cae9e

path.description

449f094

djbe mentioned this pull request Nov 26, 2016

Multiple paths #213

Merged

3 tasks

AliSoftware requested changes Nov 29, 2016

View reviewed changes

djbe added 2 commits November 29, 2016 18:43

Merge branch 'master' into feature/structured-images

20b8028

spacing

0607ca9

djbe added 2 commits November 29, 2016 20:29

use enums instead of structs for namespacing

55866e8

changelog entry

dcb6df6

djbe mentioned this pull request Nov 29, 2016

String template: use enum instead of struct #218

Merged

AliSoftware approved these changes Nov 29, 2016

View reviewed changes

AliSoftware added status: ready to merge (waiting for CI) type: enhancement labels Nov 29, 2016

Merge branch 'master' into feature/structured-images

e9641ff

djbe merged commit a51e943 into SwiftGen:master Dec 5, 2016

djbe deleted the feature/structured-images branch December 5, 2016 19:06

djbe removed the status: ready to merge (waiting for CI) label Jun 24, 2020

Images: dot syntax #206

Images: dot syntax #206

Conversation

djbe commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Nov 26, 2016 • edited Loading

AliSoftware commented Nov 26, 2016

AliSoftware left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djbe commented Nov 29, 2016

AliSoftware commented Nov 29, 2016

djbe commented Nov 29, 2016

AliSoftware commented Nov 29, 2016

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016 • edited Loading

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016

AliSoftware commented Dec 1, 2016

djbe commented Dec 1, 2016

djbe commented Nov 26, 2016 •

edited

Loading

djbe commented Dec 1, 2016 •

edited

Loading