-
Notifications
You must be signed in to change notification settings - Fork 8
Reuse identifier #16
base: master
Are you sure you want to change the base?
Reuse identifier #16
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, and thanks for your contribution!
Before reviewing the whole PR in details, here are some preliminary feedback:
to know the correct type "module.class" (both Mac and iOS) of Scenes when is a custom class
This is not necessary, as it's already present in the context and template, and handled by the template itself already (even with the current version of SwiftGen and current templates, the class is already prefixed by it's module every time)
to know the correct type "module.class" (both Mac and iOS) of Scenes when is not a custom class
When it's not a custom class, the module is UIKit
, no need for hard-coding that in the context, that should also be handled by the template itself to let people have more flexibility
- to know which Scene belongs every Segue. Each Scene has a segues array
- to know which Scene belongs every Cell. Each Scene has a reuseIdentifiers array
Those two points should probably be handled in a separate PR, that current PR addresses a little too many things at once IMHO.
public var description: String { | ||
switch self { | ||
case .popoverPresentationSegueWithoutAnchorView: | ||
return "Invalid storyboard file: A popoverPresentation type segue without " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
without…?
private var readyForFirstObject = false | ||
private var readyForConnections = false | ||
private var readyForTableView = false | ||
private var readyForCollectionView = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's way too many readyFor…
boolean properties now if we go that route. Better use a state machine e.g. an enum to tell in which subnode we are, rather than these booleans, if we have so many states now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
readyState
is first step always once but next really aren't states as inside TableViews or CollectionViews may have nested segues in its cells. Maybe readyForTableViews
and readyForCollectionView
must best check as within TableViews or CollectionViews we could have nested them inside cells then can be set readyForTableView
to false
can cause skip any of them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At this point I think it would be worth migrating the XML parsing throughout the SwiftGenKit framework to a DOM parser (like SWXMLHash for example) to make the logic more readable and maintainable.
In the first versions of SwiftGen I decided to use the built-in SAX Parser in iOS (NSXMLParser
) because I didn't want to introduce an additional dependency while the parsing logic was simple enough. But with the use cases growing and the different use cases we want to handle becoming wider, it might be time to migrate now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that it might be better then to split this up in a few separate PRs:
- one PR for switch to SWXMLHash
- one PR to implement reusable identifier
- one PR for the advanced segue features you want to add
Also, please avoid creating a whole new format/context structure and try to work within the existing structure. We have to keep existing users in mind. Let's try to keep this PR focused on reuse identifiers (cells) of any kind (table, collection, ...), and defer all other functionality (segue and scene stuff) to other PRs. |
At this point I think it would be worth migrating the XML parsing throughout the SwiftGenKit framework to a DOM parser (like SWXMLHash for example) to make the logic more readable and maintainable. So to address all that properly, the next plan of action is to make separate PRs:
The first one to migrate to a DOM parser would hopefully help making the next ones way simpler, by not relying on those Those different features must be separate, both for our sanity when reviewing to keep the review and discussion isolated, and because some discussion has already been started for some of those features in existing issues. |
@carlosypunto If you want, I can do the switch to the new xml parser so you can focus on these features. Should be able to do this during the weekend. |
Here are some information about this feature for anyone interested in making it happen References
What's nextTo make this PR happen, it might be worth as @djbe mentioned to migrate the existing code to a DOM XML parser first (for everywhere in SwiftGen when we parse XML files). This work/migration has been started in #18 Once we have that, we could resume working on this feature, which will probably become way easier to implement once we have a DOM parser rather than a SAX parser. ScheduleTbh our current focus as of now is to first release SwiftGen 5.0 which will cleanup the code and remove all the remaining old code and old template variables that we deprecated after our recent great refactoring in 4.2. SwiftGen 5.0 shouldn't take long to be released (it's mainly a matter of removing stuff that we marked as deprecated in 4.2.1) but will allow us to start add new features from a cleaner codebase afterwards instead of building on top of some legacy code. So even if not strictly necessary I think it might be worth waiting a few days or weeks until SwiftGen 5.0 is out before starting to work on that PR again? |
Starting from comments of @djbe on PR #15 I saw necessary a new form for compile info from storyboard. This PR try to solve several problems in actual compilation.
Now we can: