Skip to content
This repository has been archived by the owner on Sep 6, 2018. It is now read-only.

Refactor asset catalog code to use our own parsing instead of actool. #43

Merged
merged 5 commits into from
May 28, 2017

Conversation

djbe
Copy link
Member

@djbe djbe commented May 24, 2017

Fixes SwiftGen/SwiftGen#228.
Fixes SwiftGen/SwiftGen#287.

I chose to keep the Utils/Command.swift code, we might want it later. Or should I remove it?

@djbe djbe added this to the 2.0.0 milestone May 24, 2017
Copy link
Contributor

@AliSoftware AliSoftware left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Except the nested error type, LGTM

@@ -7,166 +7,134 @@
import Foundation
import PathKit

public enum AssetCatalogParserError: Error, CustomStringConvertible {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Prefer a nested type inside AsstsCatalogParser

@AliSoftware
Copy link
Contributor

@djbe please ensure that I didn't mess it up during my rebase (it had some conflicts) then you can merge 👍

@AliSoftware
Copy link
Contributor

After thinking about it, yeah maybe we should remove Utils/Command.swift.

  • We don't use it anymore
  • If we do need to use it again maybe it won't be the exact same way, and we can still retrieve it from an old commit if really want to anyway
  • Newcomers to the codebase might wonder why we have it if they don't know the history, might be confusing
  • We're on the verge of a Breaking version, so it's the best time to remove unneeded stuff, later will be too late
  • YAGNI

@djbe djbe merged commit ae1d55f into master May 28, 2017
@djbe djbe deleted the feature/remove-actool branch May 28, 2017 20:44
@toshi0383
Copy link

Awesome!👏

@yonaskolb
Copy link

Will this be released in SwiftGen soon? I know a forthcoming 5.0 release is in the works, but if that's a while off, maybe this could be released as a hotfix as it fixes an important bug

@AliSoftware
Copy link
Contributor

As soon as I'm back from my vacation on Monday I'll do everything to fast-track the 5.0 release (which is nearly ready and just need to be pushed but I've close to no data in Rio anyway rn so will have to wait for next week for my data and time once back in Europe)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants