Split colors parser into sub-parsers & support multiple palettes (files) #40

djbe · 2017-05-22T15:56:42Z

Currently there is a bit of logic in swiftgen that selects the right parser depending on the extension. This belongs in SwiftGenKit.

Another thing is that after this refactor, we can easily call parseFile(at:) multiple times on the generic colors parser, thus adding multiple files support. Needed for an eventual #39 implementation.

AliSoftware · 2017-05-23T19:45:22Z

What about a registration mechanism, to avoid having a hard-coded enum and switch-ing on each?

Like ColorsFileParser.register(ColorsJSONFileParser.self, forExtension: "json") etc
Would feel more like a plugin-like API and make it easy to extend in the future I think?

djbe · 2017-05-23T19:47:01Z

Good idea. For the plugin api we could have stuff like ... register(ColorsFileParser, for subcommand: "colors").

Liquidsoul · 2017-05-23T20:05:26Z

Tests/SwiftGenKitTests/ColorsCLRFileTests.swift


    let result = parser.stencilContext()
    XCTDiffContexts(result, expected: "empty.plist", sub: .colors)
  }

  func testFileWithDefaults() throws {
-    let parser = ColorsCLRFileParser()
+    let parser = ColorsFileParser()


Shouldn't we keep ColorsCLRFileParser here and in other Colors*FileTests?
Because if we do this, I have the feeling we are testing two things:

the ColorsCLRFileParser part behind ColorsFileParser

and the "parser routing" in ColorsFileParser which select the correct parser to use

It would be nice to split the specific parsing tests with the tests to select the correct file parser in ColorsFileParser.
In fact, the registration mechanism suggested by @AliSoftware would allow to test this last part quite easily IMO :)

Hmm, right. But then how do you test the parser routing, namely that it invokes the right bits?
(I have the same question for the SwiftGen CLI, where we currently don't have any tests...)

@djbe You can test the routing like this, can't you?

var parserCalled: Int = 0 class FakeParser1: ColorsFileTypeParser { func parseFile(at path: Path) throws -> [String: UInt32] { parserCalled = 1 } } class FakeParser2: ColorsFileTypeParser { func parseFile(at path: Path) throws -> [String: UInt32] { parserCalled = 2 } } ColorsFileParser.register(FakeParser1.self, forExtension: "ext1") ColorsFileParser.register(FakeParser2.self, forExtension: "ext2") ColorsFileParser.parseFile(at: Path("/foo/colors.ext1")) XCTAssertEqual(parserCalled, 1)

right, obvious in hindsight 😩

Liquidsoul · 2017-05-24T19:53:16Z

Sources/Parsers/ColorsFileParser.swift

    }
  }
 }

 // MARK: - Private Helpers

-internal func parse(hex hexString: String, key: String? = nil) throws -> UInt32 {
+func parse(hex hexString: String, key: String? = nil) throws -> UInt32 {


Shouldn't this be moved where it is used in ColorsTextFileParser?

It's also used in the XML and JSON parsers.

Liquidsoul · 2017-05-24T19:55:48Z

Sources/Parsers/ColorsFileParser.swift

 extension NSColor {
-
-  fileprivate var rgbColor: NSColor? {
+  var rgbColor: NSColor? {


Shouldn't this be moved to ColorsCLRFileParser instead of changing its scope?

Currently it may only be used in the CLR parser, but that doesn't mean it won't be used in some other future parser. I think this method is general enough to stay here, or maybe move it to some general Helper file.

AliSoftware · 2017-05-27T13:45:32Z

Sources/Parsers/ColorsFileParser.swift

      return "error: Unable to parse file. \(reason)"
+    case .unsupportedFileType(let supported):


Maybe in addition to the list of supported file types we should also print the file type and/or file name which triggered the error (especially if in the future we accept multiple files for a single invocation, would be nice to know which one is wrong)?

AliSoftware · 2017-05-27T13:47:58Z

Sources/Parsers/ColorsFileParser.swift

+    let colors = try parser.parseFile(at: path)
+
+    for (name, value) in colors {
+      self.colors[name] = value


Should we warn about duplicates / colors overriding old already-parsed colors?

We'd need a way to output to stderr then.

Yeah I'm not sure that's a problem?

Do we currently have one (for showing warnings)? Currently we never show any warnings.

I don't think we have, but it's about time we introduce one.
Probably a dedicated class on which we can call a method to log a message (warning or error) and it'll be responsible of formatting and printing it to stderr

We can then adapt the printing logic depending on some flags or auto-detection. E.g. if a specific env var only used by Xcode is set, and -o provides a destination for the generated output code (making nothing else is supposed to be printed to stdout) we can prefer print the messages as warning: inputfile:line: message on stdout. But if we're not in Xcode or there's no -o and the generated code is already printed in stdout then print the error in a more human-friendly format and on stderr.

🤔 yeah indeed probably not SwiftGenKit's job.

SwiftGenKit should only either throw (in case of fatal error) or fill an array of warning messages that consumers of the library could then inspect. Or call a callback/closure that the consumer can provide, like a delegate pattern.

Hmmm, sounds more like something of a notification pattern?

Yeah except the term notification feels like you shout to whoever wants to listen (NotificationCenter) and that's not what I want.

I'd find it more pragmatic to have something like: parser.warningHandler = { message, file, line in … } for warnings (but still throw for fatal errors)?

Sure, that could work.

To clarify, I didn't mean macOS visual notifications.

Errr me neither 😂

AliSoftware · 2017-05-27T13:52:25Z

Sources/Parsers/ColorsFileParser.swift

+
+  func register(parser: ColorsFileTypeParser.Type) {
+    for ext in parser.extensions {
+      parsers[ext] = parser


Should we warn / error in case multiple parsers declare the same extension, or should it be standard and expected that the last registered one is the one with the higher priority on that duplicate extension?

I'm also wondering if we'll always distinguish parsers only according to the file extensions (we could instead imagine a static func canParseFile(file: Path) -> Bool or something — so that the parser can also analyse the file structure to tell if it can parse the file, would allow multiple parsers to support JSON or XML extensions with different internal structure/format), but it's probably better to keep that feature for later, and only add it if we really need it one day, right? (YAGNI)

We can throw an error for now, eventually we could have multiple parsers per extension (or mime-type) and should then define some priority rules, especially in a plugin scenario.

Yeah let's do that for now. We can still improve that later and make it more flexible in the future, step by step

I think that instead of throwing an error, when we have the warning system, this should trigger a warning. But leave the error in for now until we have it.

Sure. The warning closure could be part of the PR creating the unifying parser protocol (#39) as the closure will likely be part of this protocol anyway

AliSoftware · 2017-05-27T15:00:20Z

Sources/Parsers/ColorsFileParser.swift

    guard colorSpace.colorSpaceModel != .RGB else { return self }

    return usingColorSpaceName(NSCalibratedRGBColorSpace)
  }

-  internal var hexValue: UInt32 {
+  var hexValue: UInt32 {
    let hexRed   = UInt32(round(redComponent   * 0xFF)) << 24
    let hexGreen = UInt32(round(greenComponent * 0xFF)) << 16
    let hexBlue  = UInt32(round(blueComponent  * 0xFF)) << 8
    let hexAlpha = UInt32(round(alphaComponent * 0xFF))
    return hexRed | hexGreen | hexBlue | hexAlpha


Totally not part of the PR but reviewing this, I realise that we should first call rgbColor on self before extracting components from redComponent etc. Probably worth a future PR.

We only use it in one place, and call rgbColor before it. I've changed this for now, seems overkill for a PR 😄

AliSoftware · 2017-05-27T15:05:37Z

Tests/SwiftGenKitTests/ColorsTests.swift

+@testable import SwiftGenKit
+
+final class TestFileParser1: ColorsFileTypeParser {
+  static var extensions = ["test1"]


static let 😉

AliSoftware · 2017-05-27T15:07:37Z

Tests/SwiftGenKitTests/ColorsTextFileTests.swift

-    let parser = ColorsTextFileParser()
-    try parser.parseFile(at: Fixtures.path(for: "colors.txt", sub: .colors))
+    let parser = ColorsFileParser()
+    parser.colors = try ColorsTextFileParser().parseFile(at: Fixtures.path(for: "colors.txt", sub: .colors))

    let result = parser.stencilContext()


Should we really test both the parsing AND the transformation into a StencilContext?

Shouldn't we instead just put an expectation directly on the dictionary returned by parseFile without transforming it into a stencilContext — and test that conversion of a dictionary into a stencilContext in a separate test suite?

That... might make tests much more complicated, we'd have to maintain the list of colours in code whereas right now we just diff with a plist file.

For the parsing part, I'd agree with you, but how do we ensure that we've fully parsed a file? Without having a list of colours in code, and we wouldn't be able to use the context plist files as they're for the stencilContext part. Just test if one or 2 colours are present, and a count of the number of colours?

For the stencil context part, I'd suggest using ColorsFileParser as we'd normally use it:

let parser = ColorsFileParser() parser.parseFile(at: ...) let result = parser.stencilContext()

Yeah might not be worth the hassle of maintaining all of that just for making tests more unitary. Maybe we can re discuss that in a separate PR.

AliSoftware · 2017-05-27T15:08:12Z

Sources/Parsers/ColorsFileParsers/ColorsCLRFileParser.swift

+import PathKit
+
+final class ColorsCLRFileParser: ColorsFileTypeParser {
+  static var extensions = ["clr"]


static let

djbe · 2017-05-30T21:52:09Z

Needs SwiftGen/templates#55 to align submodule

…stem

AliSoftware · 2017-06-02T19:59:41Z

Sources/Parsers/ColorsFileParser.swift

 public enum ColorsParserError: Error, CustomStringConvertible {
+  case duplicateExtensionParser(ext: String, existing: String, new: String)
  case invalidHexColor(string: String, key: String?)


Mmmmh maybe we should add a path: Path associated value to this one too to make it homogeneous?

AliSoftware · 2017-06-02T20:03:31Z

Sources/Parsers/ColorsFileParsers/ColorsTXTFileParser.swift

+    let whitespace = CharacterSet.whitespaces
+    let skippedCharacters = NSMutableCharacterSet()
+    skippedCharacters.formUnion(with: whitespace)
+    skippedCharacters.formUnion(with: skippedCharacters as CharacterSet)


I know this code was already there before, but re-reading this, I can't understand what this does… a union with itself?!

Hmm, it was added in this commit:
SwiftGen/SwiftGen@ea9c97f
I can't see why though.

Seems to work fine without, so I'll remove it

djbe added this to the 2.0.0 milestone May 22, 2017

djbe added the type: internal label May 22, 2017

djbe requested review from AliSoftware and Liquidsoul May 23, 2017 19:34

AliSoftware mentioned this pull request May 23, 2017

Multiple strings tables support #41

Merged

Liquidsoul reviewed May 23, 2017

View reviewed changes

Liquidsoul reviewed May 24, 2017

View reviewed changes

AliSoftware suggested changes May 27, 2017

View reviewed changes

AliSoftware force-pushed the feature/split-color-parser branch from 1f3f6f1 to 5aebc67 Compare May 27, 2017 20:58

djbe force-pushed the feature/split-color-parser branch from 5aebc67 to 7ae20f5 Compare May 28, 2017 20:52

AliSoftware mentioned this pull request May 30, 2017

Parser protocol #39

Closed

djbe force-pushed the feature/split-color-parser branch 2 times, most recently from 0c0e9b1 to fb9dd25 Compare May 30, 2017 21:42

djbe changed the title ~~Split colors parser into multiple sub-parsers~~ Split colors parser into sub-parsers & support multiple palettes (files) May 30, 2017

djbe mentioned this pull request May 30, 2017

Colors: support multiple palettes SwiftGen/templates#55

Merged

AliSoftware approved these changes May 30, 2017

View reviewed changes

djbe added 2 commits May 31, 2017 11:59

Split colors parser into multiple sub-parsers using a registration sy…

66809c5

…stem

Changelog entry

ee51916

djbe force-pushed the feature/split-color-parser branch from ced4247 to 2ebc5d4 Compare May 31, 2017 10:01

AliSoftware reviewed Jun 2, 2017

View reviewed changes

AliSoftware approved these changes Jun 2, 2017

View reviewed changes

djbe added 4 commits June 4, 2017 20:02

Add support for multiple color palettes

bcfb6ee

Update documentation

4f462c1

remove some old code

10e3a24

add path to invalidHexColor error

8b6e987

djbe force-pushed the feature/split-color-parser branch from 3938c3e to 8b6e987 Compare June 4, 2017 18:02

djbe merged commit df6219d into master Jun 4, 2017

djbe deleted the feature/split-color-parser branch June 4, 2017 20:59

		return "error: Unable to parse file. \(reason)"
		case .unsupportedFileType(let supported):

Split colors parser into sub-parsers & support multiple palettes (files) #40

Split colors parser into sub-parsers & support multiple palettes (files) #40

Conversation

djbe commented May 22, 2017

AliSoftware commented May 23, 2017

djbe commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware May 23, 2017 • edited Loading

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 May 24, 2017 • edited Loading

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 May 27, 2017 • edited Loading

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

djbe commented May 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AliSoftware May 23, 2017 •

edited

Loading

djbe May 24, 2017 •

edited

Loading

AliSoftware May 27, 2017 •

edited

Loading