Add ability to set an order of groups #613

Beniamiiin · 2019-07-23T08:53:49Z

I implemented the option called groupOrder. It's for setting an order of groups. Currently, we can just set the groupSortPosition, but sometimes we wanna set directly order of groups.

Some examples

Set the order of the main group, i.e. the project.

I have a project with a structure:

Test.xcodeproj
  Sources
  Resources
  Tests
  Configurations
  Products
  Frameworks

Without groupOrder option it will be sorted alphabetically, to set the order as above we can use the new option in this way:

options:
  groupsOrder: 
    - order: [Sources, Resources, Tests, Support files, Configurations]

Set the order of groups whose names end with Screen.

I have a group with a structure:

Test.xcodeproj
  Sources
    PresentationLayer
      SplashScreen
        View
        Presenter
        Interactor
        Entities
        Assembly
  Resources
  Tests
  Configurations
  Products
  Frameworks

With out groupOrder option, it will be sorted alphabetically, to set the order as above we can use the new option in this way:

options:
  groupsOrder:
    - order: [Sources, Resources, Tests, Support files, Configurations]
    - pattern: '^.*Screen$'
      order: [View, Presenter, Interactor, Entities, Assembly]

brentleyjones · 2019-07-23T13:48:41Z

Is this only at the top level? How about being able to do it on sub-levels as well? Maybe that is covered by your second example, I'm not sure.

Beniamiiin · 2019-07-23T14:16:53Z

Is this only at the top level? How about being able to do it on sub-levels as well? Maybe that is covered by your second example, I'm not sure.

No, it's work on any level and you are right, it's covered by the second example. I'll update the example.

yonaskolb

Thanks for the PR @Beniamiiin. How important is it to your team to have this sorting? Both for the top level and for sub groups?

Sources/XcodeGenKit/PBXProjGenerator.swift

Sources/ProjectSpec/StringExtensions.swift

yonaskolb · 2019-08-02T02:11:55Z

Docs/ProjectSpec.md

@@ -114,6 +114,7 @@ Note that target names can also be changed by adding a `name` property to a targ
  - `none` - sorted alphabetically with all the other files
  - `top` - at the top, before files
  - `bottom` (default) - at the bottom, after other files
+- [ ] **groupsOrder**: **[[GroupOrder]](#groupOrder)** - An order of groups.


Not in love with the name groupsOrder, but I also can't think of something better right now.

Not only you, I don't like it too, but I couldn't find more lovely name :( I'll try to think about it again.

How about groupOrdering?

yonaskolb · 2019-08-02T02:14:54Z

Sources/XcodeGenKit/PBXProjGenerator.swift

-                } else {
-                    if child1.nameOrPath != child2.nameOrPath {
-                        return child1.nameOrPath.localizedStandardCompare(child2.nameOrPath) == .orderedAscending
+        if let groupOrder = project.options.groupsOrder.first(where: { group.nameOrPath == $0.pattern || group.nameOrPath.isMatch(to: $0.pattern) }) {


nameOrPath evaluates to name ?? path ?? "". That means groups who have a different path than just the name won't be supported.
In either case doing something like Group1/Group2/* won't work as the name is just the end and the path is a relative path.

Okay, so should I to use just name?

@yonaskolb I need your help. I don't understand what is the correct way to do it.

brentleyjones · 2019-08-02T13:36:36Z

How important is it to your team to have this sorting? Both for the top level and for sub groups?

I'll say for us that the top level is pretty important. We've gone the route of adding 1., 2. , etc. to the directory names to get the sorting. For context, we are grouping similar targets into directories (applications, experiences, capabilities, components, services, core frameworks, etc.), since we have a massive number of targets.

Beniamiiin · 2019-08-05T06:34:06Z

Thanks for the PR @Beniamiiin. How important is it to your team to have this sorting? Both for the top level and for sub groups?

For us, it's important because currently, we use my fork with the implementation of this feature.

brentleyjones · 2019-08-29T16:38:05Z

@Beniamiiin @yonaskolb What is needed to move this forward? We personally would love this feature landed.

Beniamiiin · 2019-08-30T05:09:06Z

@brentleyjones firstly I need some help from @yonaskolb to fix all review issues and secondly @yonaskolb should review the PR again.

Beniamiiin · 2019-09-19T06:07:40Z

@yonaskolb are we gonna do something with this PR?

Beniamiiin · 2020-03-29T09:20:36Z

@yonaskolb are we gonna do something with this PR?

brentleyjones · 2020-03-30T13:50:42Z

I'm still a big fan of this change as well. @yonaskolb, since you had some PR comments I'll leave it to you to re-review. Thanks!

Beniamiiin · 2020-04-03T08:43:06Z

@yonaskolb I'm so sorry can you give us some information about this PR?

brentleyjones

Can you add some test fixtures? Showing how it doesn't affect ordering when the property isn't declared, and some that covers all of the functionality listed.

I'll give this a spin locally as well. If nothing comes up there, after the changes we should be good to go.

brentleyjones · 2020-04-03T12:54:07Z

Docs/ProjectSpec.md

@@ -114,6 +114,7 @@ Note that target names can also be changed by adding a `name` property to a targ
  - `none` - sorted alphabetically with all the other files
  - `top` - at the top, before files
  - `bottom` (default) - at the bottom, after other files
+- [ ] **groupsOrder**: **[[GroupOrder]](#groupOrder)** - An order of groups.


How about groupOrdering?

brentleyjones · 2020-04-03T13:17:06Z

It runs nicely with our project, so just looking for the requested changed and we should be good to go.

yonaskolb · 2020-04-07T08:19:08Z

Sources/XcodeGenKit/PBXProjGenerator.swift

+            for groupName in order {
+                guard let group = groups.first(where: { $0.nameOrPath == groupName }) else {
+                    continue
+                }
+
+                filteredGroups.append(group)
+                groups.removeAll { $0 == group }
+            }


I think this will cause performance issues in large projects that use a regex pattern, as there are a lot of nested loops here, but I guess it's up to the project author to take on that cost

Sources/XcodeGenKit/PBXProjGenerator.swift

Docs/ProjectSpec.md

Sources/XcodeGenKit/PBXProjGenerator.swift

brentleyjones · 2020-04-15T18:19:36Z

@Beniamiiin: after @yonaskolb's feedback is addressed and this is rebased on master, we can merge it. Thanks!

Beniamiiin · 2020-04-16T05:31:04Z

@Beniamiiin: after @yonaskolb's feedback is addressed and this is rebased on master, we can merge it. Thanks!

@brentleyjones I'll do it at this weekend.

# Conflicts: # CHANGELOG.md

Beniamiiin · 2020-04-26T11:20:42Z

@yonaskolb can you review it again please

brentleyjones

Since this feature is gated behind people using it, and it works in it's current state, I say we merge this and address any issues in followup PRs.

mthole · 2020-08-13T15:44:56Z

@Beniamiiin Found this feature today. Very useful, thank you!

Beniamin Sarkisyan added 4 commits July 23, 2019 11:22

Add groupOrder option for set up an order of groups

58f4260

Update docs

2225c9e

Update CHANGELOG.md

3ead44b

Fix merge issues

9eb9638

yonaskolb reviewed Aug 2, 2019

View reviewed changes

Fix review issues

63bb6ad

Merge branch 'master' into groups_order_option

de5e0e9

brentleyjones reviewed Apr 3, 2020

View reviewed changes

Beniamin Sarkisian added 2 commits April 7, 2020 03:49

Merge branch 'master' into groups_order_option

b9da574

Rename groupsOrder to groupOrdering

2448168

yonaskolb reviewed Apr 7, 2020

View reviewed changes

Sources/XcodeGenKit/PBXProjGenerator.swift Outdated Show resolved Hide resolved

Beniamin Sarkisian added 2 commits April 25, 2020 16:30

Fix review issues

9e174f8

Merge branch 'master' into groups_order_option

cb1b079

# Conflicts: # CHANGELOG.md

Remove debug code

9ffa271

brentleyjones approved these changes Apr 28, 2020

View reviewed changes

brentleyjones merged commit 0ad02b7 into yonaskolb:master Apr 28, 2020

jeffctown mentioned this pull request Apr 29, 2020

Feature/catch xcode test failures #851

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ability to set an order of groups #613

Add ability to set an order of groups #613

Beniamiiin commented Jul 23, 2019 •

edited

Loading

brentleyjones commented Jul 23, 2019 •

edited

Loading

Beniamiiin commented Jul 23, 2019 •

edited

Loading

yonaskolb left a comment

yonaskolb Aug 2, 2019

Beniamiiin Aug 5, 2019

brentleyjones Apr 3, 2020

yonaskolb Aug 2, 2019

Beniamiiin Aug 5, 2019

Beniamiiin Aug 6, 2019

brentleyjones commented Aug 2, 2019

Beniamiiin commented Aug 5, 2019

brentleyjones commented Aug 29, 2019 •

edited

Loading

Beniamiiin commented Aug 30, 2019

Beniamiiin commented Sep 19, 2019

Beniamiiin commented Mar 29, 2020

brentleyjones commented Mar 30, 2020

Beniamiiin commented Apr 3, 2020

brentleyjones left a comment

brentleyjones Apr 3, 2020

brentleyjones commented Apr 3, 2020

yonaskolb Apr 7, 2020

brentleyjones commented Apr 15, 2020 •

edited

Loading

Beniamiiin commented Apr 16, 2020

Beniamiiin commented Apr 26, 2020

brentleyjones left a comment

mthole commented Aug 13, 2020

Add ability to set an order of groups #613

Add ability to set an order of groups #613

Conversation

Beniamiiin commented Jul 23, 2019 • edited Loading

brentleyjones commented Jul 23, 2019 • edited Loading

Beniamiiin commented Jul 23, 2019 • edited Loading

yonaskolb left a 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

brentleyjones commented Aug 2, 2019

Beniamiiin commented Aug 5, 2019

brentleyjones commented Aug 29, 2019 • edited Loading

Beniamiiin commented Aug 30, 2019

Beniamiiin commented Sep 19, 2019

Beniamiiin commented Mar 29, 2020

brentleyjones commented Mar 30, 2020

Beniamiiin commented Apr 3, 2020

brentleyjones left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brentleyjones commented Apr 3, 2020

Choose a reason for hiding this comment

brentleyjones commented Apr 15, 2020 • edited Loading

Beniamiiin commented Apr 16, 2020

Beniamiiin commented Apr 26, 2020

brentleyjones left a comment

Choose a reason for hiding this comment

mthole commented Aug 13, 2020

Beniamiiin commented Jul 23, 2019 •

edited

Loading

brentleyjones commented Jul 23, 2019 •

edited

Loading

Beniamiiin commented Jul 23, 2019 •

edited

Loading

brentleyjones commented Aug 29, 2019 •

edited

Loading

brentleyjones commented Apr 15, 2020 •

edited

Loading