Switch the asset catalog parser to using "actool" to parse its contents#199
Switch the asset catalog parser to using "actool" to parse its contents#199djbe merged 15 commits intoSwiftGen:masterfrom djbe:feature/images-actool
Conversation
…g "actool" to parse its contents
AliSoftware
left a comment
There was a problem hiding this comment.
Looks like a good start, thanks for the PR 👍
| let filename = item[AssetCatalog.filename.rawValue] as! NSString | ||
|
|
||
| // this is a simple imageset | ||
| if filename.pathExtension == AssetsCatalogParser.ImageSet { |
There was a problem hiding this comment.
SwiftGen has a dependency to PathKit which is a pod providing Swiftier API for manipulating files, file extensions, basenames, etc.
You should switch your code so it uses it (instead of having to convert to NSString and use Foundation) 😉
| return children | ||
| } | ||
|
|
||
| private func executeCommand(command: String, args: [String]) -> NSData { |
There was a problem hiding this comment.
Wondering if that couldn't be useful for other subcommands one day so maybe worth moving to some helper?
There was a problem hiding this comment.
Sure, where should I put it? A new subfolder in GenumKit called Utils next to Parsers and Stencil?
There was a problem hiding this comment.
Good question… yeah sounds about right!
AliSoftware
left a comment
There was a problem hiding this comment.
We should add unit tests for the use case you claim that this PR solved as a side effect of using actool (I believe you were mentioning namespaces with spaces in their names or something?)
Otherwise seems good (even if I only reviewed from my phone on GitHub)
| } | ||
|
|
||
| extension AssetsCatalogParser { | ||
| static let ImageSet = "imageset" |
There was a problem hiding this comment.
I'd rename that constant imageSetExtension (_lower_CsmelCase seems to be the rule even for constants in Swift I think)
|
|
||
| extension AssetsCatalogParser { | ||
| private func loadAssetCatalogContents(path: String) -> [[String: AnyObject]]? { | ||
| let command = Command("xcrun", arguments: "actool", "--print-contents", path) |
There was a problem hiding this comment.
Nitpicking: indentation
|
Oh sorry I re-read your original comment you were just talking about namespaces in the asset catalog, which you already covered in your additional tests, right? |
|
Jup, I've added folders in the current asset catalog, both namespaced and normal ones. |
|
Still missing a CHANGELOG entry though 😜 |
|
Gah, gimme a second |
CHANGELOG.md
Outdated
| [David Jennes](https://github.com/djbe), [#175](https://github.com/AliSoftware/SwiftGen/pull/175) | ||
| * Remove the `key` param label from the `tr` function for Localized String in the swift3 template [AndrewSB](https://github.com/AndrewSB), [#190](https://github.com/AliSoftware/SwiftGen/pull/190) | ||
| * The `swiftgen images` command now uses the `actool` utility to parse asset catalogs, | ||
| ensuring that the parser correctly handles namespaced folders. |
There was a problem hiding this comment.
You forgot to add two spaces after the full stop (which will allow the rendered markdown to insert a newline in the same paragraph here)
There was a problem hiding this comment.
I know, I'm pedantic ^^
There was a problem hiding this comment.
I thought I copied my previous changelog, seems I didn't, or Xcode is messing with me.
There was a problem hiding this comment.
Wait… in fact a lot of those CHANGELOG entry are not correctly formatted 😱
There was a problem hiding this comment.
I noticed 😄 I can fix that in one commit if you want?
There was a problem hiding this comment.
Yeah that would be nice 😉👍
There was a problem hiding this comment.
There typical expected format is the one used in 3.0.1 section and before.
* Description of the change.••
[Author Name](GH profile link)
[#PRNum](link to PR)
With • to be replaced by a space, otherwise going on a new, indented line for every link after the description because I have OCD 😂
djbe
left a comment
There was a problem hiding this comment.
herp derp, I'm going to sleep, I'm making a mess of the commit history 😐
There was a problem hiding this comment.
Ok, LGTM now 👍
I'll let you guys @djbe @dostrander decide when to the merge (probably after your good night sleep 😜), as you seem to already have started a list of PRs and milestones, but you have my go 👌
This is a first step to eventually implementing the dot-syntax for asset catalogs, mentioned in #52.
The good thing about this is that, without changing the template, it already fixes an issue with the generated code for catalogs with namespaces folders. Currently the generated enum cases only contain the last filename of imagesets (eg.
case myImage = "myImage"), but if they are contained in namespaced folders, the case should actually becase namespace1_namespace2_myImage = "namespace1\namespace2\myImage".This is a preliminary pull request, I still need to add a changelog entry and a bunch of unit tests. But I'd like a quick review of the changed code and your input before continuing.