Switch to DOM-based XML parser #18
Conversation
djbe
force-pushed
the
feature/new-xml-parser
branch
from
1147548
to
943240d
Compare
djbe
force-pushed
the
feature/new-xml-parser
branch
from
943240d
to
4f62ed2
Compare
| @@ -28,90 +28,70 @@ public final class StoryboardParser { | |||
| let customModule: String? | |||
| } | |||
|
|
|||
| enum XMLScene { | |||
There was a problem hiding this comment.
This is the only bit I'm not too happy with. I first had an XML enum, with inside it separate enums for paths, tags and attributes, but then swiftlint started complaining about nested types. ¯\_(ツ)_/¯
There was a problem hiding this comment.
Yeah, not that big a deal imho, as long as we use …Path, …Tag & …Attribute suffixes, we don't have that many constants here to justify having structured constants at all cost, that's ok
djbe
force-pushed
the
feature/new-xml-parser
branch
2 times, most recently
from
c21d73d
to
22fa6b3
Compare
djbe
force-pushed
the
feature/new-xml-parser
branch
from
22fa6b3
to
80f5beb
Compare
djbe
force-pushed
the
feature/new-xml-parser
branch
2 times, most recently
from
4e67c50
to
9929356
Compare
djbe
force-pushed
the
feature/new-xml-parser
branch
from
9929356
to
34f2143
Compare
| throw ColorsParserError.invalidFile(reason: reason) | ||
| for color in document.xpath(XML.colorPath) { | ||
| let value = color.stringValue | ||
| guard let name = color["name"], !name.isEmpty else { continue } |
There was a problem hiding this comment.
Shouldn't we throw an error if the name attribute is missing or empty?
There was a problem hiding this comment.
We probably should. My initial try for this was to keep functionality as it is, but we might as wel fix some stuff while we're at it.
| scenes.insert(Scene(storyboardID: id, | ||
| tag: scene.tag ?? "", | ||
| customClass: customClass, | ||
| customModule: customModule)) |
There was a problem hiding this comment.
I know we didn't consider that case in the old code either, but what if (for some reason or maybe a malformed Storyboard) we have a scene which have no storyboardIdentifier attribute, is it normal that we skip it? It shouldn't happen in normal cases I think, but given how Xcode generate some strange stuff with storyboards sometimes (see the issue with customModule in the other PR), maybe consider it? I don't know what would be best in that case though (ignoring it or generating a constant with an empty storyboardID?) or if that really matters or if I'm just nitpicking as often 😉
There was a problem hiding this comment.
Scenes can have no storyboardIdentifier, in that case they're anonymous scenes. Usually such scenes are connected via segues to other scenes. Anonymous scenes cannot be directly instantiated from code, exactly because they don't have an identifier.
In all my projects, most of my scenes don't have an identifier exactly because they're all connected via segues. Only some of them have an identifier, and those correctly appear in the swiftgen generated file.
| @@ -28,90 +28,70 @@ public final class StoryboardParser { | |||
| let customModule: String? | |||
| } | |||
|
|
|||
| enum XMLScene { | |||
| static let path = "/document/scenes/scene/objects/*[@sceneMemberID=\"viewController\"]" | |||
There was a problem hiding this comment.
I'd maybe rather name that constant sceneXPath rather than just path?
Also, using @sceneMemberID is a new logic, isn't it? We didn't use the sceneMemberID to identify scenes in the previous SAX version of our parsing did we? Maybe that's all right but just want to make sure we're not over-restricting there in case there's some use case and strange storyboards out there which don't have that attribute on scenes
There was a problem hiding this comment.
You're right that it's new logic, before we were just taking the first item in the objects array. But that actually lead to issue #17, where sometimes viewController isn't the first item in the array. Instead we now search for the actual viewcontroller in that logic, thus the @sceneMemberID.
| @@ -28,90 +28,70 @@ public final class StoryboardParser { | |||
| let customModule: String? | |||
| } | |||
|
|
|||
| enum XMLScene { | |||
There was a problem hiding this comment.
Yeah, not that big a deal imho, as long as we use …Path, …Tag & …Attribute suffixes, we don't have that many constants here to justify having structured constants at all cost, that's ok
| let value = color.stringValue | ||
| guard let name = color["name"], !name.isEmpty else { continue } | ||
| guard let name = color["name"], !name.isEmpty else { | ||
| throw ColorsParserError.invalidFile(reason: "Invalid structure, color must have a name.") |
There was a problem hiding this comment.
"… color \(value) must have a name …" to help identify in the error message which line on the XML we're taking about?
There was a problem hiding this comment.
You want to add the xml line number? I don't think we have access to that.
Edit: Nvm, you want to display the parsed value, gotcha.
There was a problem hiding this comment.
Yeah just inserting the value in the error message 😉 (sorry it wasn't super clear ^^)
| scenes.insert(Scene(storyboardID: id, | ||
| tag: scene.tag ?? "", | ||
| customClass: customClass, | ||
| customModule: customModule)) |
djbe
force-pushed
the
feature/new-xml-parser
branch
from
b438714
to
403403c
Compare
Tried SWXMLHash, but it doesn't support XPath queries, which removes one of the main advantages of DOM-based parsing, and is needed for finding
segueconnections in a storyboard.We could use the built in
(NS)XMLDocumentparser, but it has a higher memory footprint, and antiquated syntax. Still, if we don't want the dependency, we can always switch to the built in Foundation parser.Fixes #17.