Fix catalog entry sort by fdiaz · Pull Request #57 · SwiftGen/SwiftGenKit

fdiaz · 2017-10-05T18:22:30Z

Closes #56

AliSoftware · 2017-10-05T18:25:34Z

Note that this is a duplicate of #55 so whichever of those two will be validated and finished first will invalidate the other one.

PS: If you wish, it could be a nice courtesy to also credit the original author of #55 in your CHANGELOG entry too if you used #55 as inspiration and this #56 is just a continuation of their work to finish the work 😉

fdiaz · 2017-10-05T18:28:23Z

You're totally right @AliSoftware ! Updated the CHANGELOG. I didn't wanna build on top of #55 since this is a slightly different approach.

#55 modifies AssetsCatalogContext.swift and this PR modifies AssetsCatalogParser.swift.

I took this road since the underlying problem is with the children() function. So this sorts the ouput of anyone using process(folder: Path, withPrefix prefix: String = "")

AliSoftware · 2017-10-05T18:30:28Z

+final class CatalogEntryTests: XCTestCase {
+  func testGroupEntry_IsComparableAlphabetically() {
+    let entryA = Catalog.Entry.group(name: "A", items: [])
+    let entryB = Catalog.Entry.group(name: "B", items: [])


Could be nice to also add some tests using different letter case to check if that comparability is case-sensitive or not (but whether it is or not, that it always sorts ["A","a"].map { Catalog.Entry.group(name: $0, items: []) } in the same order

Now that I think about it, I should also test that two groups with the same name order its items correctly.

I'll work on that 👍

AliSoftware · 2017-10-05T18:33:44Z

+    let entryA = Catalog.Entry.group(name: "A", items: [])
+    let entryB = Catalog.Entry.group(name: "B", items: [])
+
+    XCTAssertEqual(entryA < entryB, true)


The expected usage of XCTAssertEqual is to take 2 parameters to equate, like XCTAssertEqual(entryA, entryB).

If you want to check if an expression, like entryA < entryB, is true, prefer XCTAssert or XCTAssertTrue.
(Some even prefer using XCTAssertLessThan(entryA, entryB) but this last one being less commonly known, XCTAssert or XCTAssertTrue are generally more fitting)

AliSoftware · 2017-10-06T08:56:03Z

+      return .orderedDescending
+    }
+
+    return compare(Array(lhs.dropFirst()), Array(rhs.dropFirst()))


I'm not a fan of using this recursive approach as it can make the stack grow quite quickly for a group containing 100s of images…

What about:

private static func compare(_ lhs: [Catalog.Entry], _ rhs: [Catalog.Entry]) -> ComparisonResult { for pair in zip(lhs, rhs) { if pair.0 < pair.1 { return .orderedAscending } if pair.0 > pair.1 { return .orderedDescending } } // If we arrive a this point, this means all pairs have been considered equal // but one of the list might be longer than the other if lhs.count < rhs.count { return .orderedAscending } if lhs.count > rhs.count { return .orderedDescending } return .orderedSame }

That's a good point and a much cleaner implementation. Updating 👍

I even wonder if the last part (last 3 lines) couldn't be reduced to return compare(lhs.count, rhs.count)?
(Don't we have a compare(Int,Int) -> ComparisonResult in the stdlib?)

AliSoftware · 2017-10-06T13:21:26Z

@@ -82,7 +133,7 @@ extension AssetsCatalogParser {
  func process(folder: Path, withPrefix prefix: String = "") -> [Catalog.Entry] {
    return (try? folder.children().flatMap {


@fdiaz I just realised: wouldn't it be simpler instead of all the Equatable + Comparable implementations you added… to just sort the output of children() directly? i.e. apply folder.children().sorted(by: <).flatMap { … } to children, instead of using folder.children().flatMap { … }.sorted(by: <) and sort the the final, recursively-constructed structure?

Which would mean in practice removing all the code that you added above and a simpler and mode performant solution (because you don't have to traverse the recursive structure twice) in the end, right?

Oh wow, you're right. I didn't notice that Path was already Comparable. I'll just go back with all these changes and add that little change instead. Nice catch!

The children() function uses under the hood contentsOfDirectory(atPath:) https://developer.apple.com/documentation/foundation/filemanager/1414584-contentsofdirectory This array returned by that function has an undefined order.

djbe

We should probably also change our tests to run on the latest Xcode, and the latest macOS (if possible), to ensure that this is actually fixed (and remains so).

djbe · 2017-10-08T18:00:35Z

 ### Bug Fixes

-_None_
+* Fixes an issue in High Sierra where the output of the processed Catalog Entries was not ordered alphabetically.


This seems to be missing the 2 spaces at the end?

AliSoftware · 2017-10-08T21:12:33Z

Good point @djbe our scripts would probably need updating, or more precisely the .circle.yml configuration should target 10.13 at some point.

@fdiaz any news on the rebase we spoke about on slack simplifying this whole PR?

djbe · 2017-10-08T21:15:25Z

What simplification? This PR boils down to 1 line of code 😄

AliSoftware · 2017-10-08T21:18:03Z

Oh I missed the notification telling me that @fdiaz did in fact already commit the simplification we talked about about sorting children() directly (idk if you saw the PR before that @djbe but it was sorting the output of process() instead, having to do a recursive sort and having to implement Equatable+Comparable on Catalog.Entry and all.

Seems like the Circle tests are still failing though.

djbe · 2017-10-08T21:33:52Z

Yeah, I partially followed it, will fix the contexts.

nafu · 2017-10-09T06:36:30Z

Cool 💚

fdiaz · 2017-10-09T09:33:21Z

Thanks for finishing this up @djbe 👍

I'm not sure why the tests were still failing though? Any context on that? I was about to take a look on High Sierra to see what was going on with that.

djbe · 2017-10-09T10:39:37Z

There is a slight difference between the (built in) sort-order in Sierra, and the sorting we now apply. Nothing much to worry about.

fdiaz force-pushed the fix/catalog-entry-sort branch 2 times, most recently from 60adef8 to 42f14e5 Compare October 5, 2017 18:23

fdiaz force-pushed the fix/catalog-entry-sort branch from 42f14e5 to aafe853 Compare October 5, 2017 18:27

AliSoftware reviewed Oct 5, 2017

View reviewed changes

AliSoftware reviewed Oct 6, 2017

View reviewed changes

fdiaz force-pushed the fix/catalog-entry-sort branch 2 times, most recently from f4463e2 to bc03531 Compare October 6, 2017 11:38

AliSoftware approved these changes Oct 6, 2017

View reviewed changes

AliSoftware requested a review from djbe October 6, 2017 11:51

AliSoftware suggested changes Oct 6, 2017

View reviewed changes

fdiaz added 2 commits October 6, 2017 12:18

Add CHANGELOG entry

c28d130

Sort children output when parsing folder

5d1401f

The children() function uses under the hood contentsOfDirectory(atPath:) https://developer.apple.com/documentation/foundation/filemanager/1414584-contentsofdirectory This array returned by that function has an undefined order.

fdiaz force-pushed the fix/catalog-entry-sort branch from bc03531 to 5d1401f Compare October 6, 2017 20:28

djbe suggested changes Oct 8, 2017

View reviewed changes

djbe added 2 commits October 8, 2017 23:51

align submodule

608f2e6

spaces

7128771

djbe force-pushed the fix/catalog-entry-sort branch from cddee57 to 7128771 Compare October 8, 2017 21:51

djbe mentioned this pull request Oct 8, 2017

Fix catalog entry sort SwiftGen/templates#82

Merged

AliSoftware merged commit 1621d9d into SwiftGen:master Oct 8, 2017

AliSoftware mentioned this pull request Oct 8, 2017

Revise entry name order in Assets Catalog #55

Closed

fdiaz deleted the fix/catalog-entry-sort branch October 9, 2017 09:32

AliSoftware mentioned this pull request Oct 10, 2017

Non-alphabetical ordering on APFS/High Sierra SwiftGen/SwiftGen#339

Closed

		@@ -82,7 +133,7 @@ extension AssetsCatalogParser {
		func process(folder: Path, withPrefix prefix: String = "") -> [Catalog.Entry] {
		return (try? folder.children().flatMap {

Conversation

fdiaz commented Oct 5, 2017

Uh oh!

AliSoftware commented Oct 5, 2017

Uh oh!

fdiaz commented Oct 5, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AliSoftware Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

fdiaz Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

AliSoftware Oct 5, 2017

Choose a reason for hiding this comment

Uh oh!

AliSoftware Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

fdiaz Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

AliSoftware Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

AliSoftware Oct 6, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

fdiaz Oct 6, 2017

Choose a reason for hiding this comment

Uh oh!

djbe left a comment

Choose a reason for hiding this comment

Uh oh!

djbe Oct 8, 2017

Choose a reason for hiding this comment

Uh oh!

AliSoftware commented Oct 8, 2017

Uh oh!

djbe commented Oct 8, 2017

Uh oh!

AliSoftware commented Oct 8, 2017

Uh oh!

djbe commented Oct 8, 2017

Uh oh!

nafu commented Oct 9, 2017

Uh oh!

fdiaz commented Oct 9, 2017

Uh oh!

djbe commented Oct 9, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

fdiaz commented Oct 5, 2017 •

edited

Loading

fdiaz Oct 6, 2017 •

edited

Loading

AliSoftware Oct 6, 2017 •

edited

Loading