Fix NSString compile error on linux

[rahul0x24]

Reference: https://stackoverflow.com/questions/37293388/cannot-convert-value-of-type-string-to-type-nsstring-in-coercion-when-i-use

[djbe]

I think a better solution would be to directly use the PathKit methods instead of casting to NSString.

[rahul0x24]

@djbe Sorry, I was in a rush to solve the compilation issue that I didn't see what the function was about. You may discard this pr if you want to solve it using PathKit.

[djbe]

Oh no, you can just update your PR to create a Path from the string, and call the corresponding methods for each filter (lastComponent and parent().normalize()).
It's just a more robust solution long-term to use PathKit instead of the old NSString methods.

Note: don't forget to add a changelog entry for your changes, see https://github.com/SwiftGen/SwiftGen/blob/master/CONTRIBUTING.md#requirements--conventions.

[rahul0x24]

@djbe PathKit is internally using the same NSString methods and StencilSwiftKit doesn't depend on PathKit directly right now but through Stencil.

I still think that the better solution would be to go with NSString than PathKit as there are only two methods. Maybe when there is a bigger use case where a new file Filter+Paths.swift is required, then it might make sense to go with PathKit and even add that as a dependency in Package.swift.

If you still think PathKit is a better solution, I am happy to update the PR.

Let me know what you think.

[AliSoftware]

Even if StencilSwiftKit doesn't depend directly on Stencil, it still depends on it so it will always have access to PathKit's methods to manipulate paths like that. So no need to add it to Package.swift explicitly, that's a transitive dependency already.
So I think it's still worth it to use PathKit methods directly, we have access to them already anyway.

[rahul0x24]

@AliSoftware Sure!. I will update the PR to use PathKit methods then.

[rahul0x24]

One of the test fails when calculating dirname with Input(string: "scratch.tiff"): ""
XCTAssertEqual failed: ("Optional(".")") is not equal to ("Optional("")")

According to https://linux.die.net/man/1/dirname, Its the expectation which is wrong.

[AliSoftware]

Behavior on dirname("somefile.ext") might differ between Linux and macOS i practice maybe (ie we'd have to check how that's implemented in macOS / Foundation but might be different from the Linux spec).

If they are the same and both return "." on all platforms then there's no issue and your recent commit will fix that.

If they are different in practice today just because they are based on different methods for each OS and those OS methods (on which we don't really have control over) behave differently too, I don't know what's the best approach (respect the convention depending on the OS, like "." for Linux but "" for macOS? Or uniformize to "." on all platforms even if it means treating the special case ourselves on the macOS implementation?)

Anyway, we have to ensure that we make the tests pass on all platforms and not just one (I don't remember if CircleCI is already configured to test on all platforms on this project already?)

[djbe]

We test stencilswiftkit on linux, we don't test swiftgen yet (because SPM reasons).

I think matching the platform behaviour would be best? We'll have to add a platform check in the tests (#if ...), but it'll be minimal.

[rahul0x24]

@AliSoftware @djbe The behaviour of dirname is the same on both macOS and Linux platforms and both return '.' on dirname("somefile.ext").

[djbe]

Oh, then just update the test 😄

[rahul0x24]

@djbe Let me know if there are any more changes required. Everything seems to pass now ✌🏻

[djbe]

Cool, thank you for your contribution!