Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

apply_change_to_project: fix group moves. #88

Merged
merged 1 commit into from
Sep 12, 2022
Merged

Conversation

ashdnazg
Copy link
Member

@ashdnazg ashdnazg commented Sep 1, 2022

No description provided.

Copy link
Collaborator

@byohay byohay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

round.

@@ -195,12 +195,17 @@ def apply_group_and_file_diffs(project, diffs)
end

def apply_group_removals(project, removals)
removals.each do |change, path|
removals.reverse.each do |change, path|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like it that it relies on the assumption that the inner groups come after the outer ones. I don't know if it's better, but since we know that only file changes were already removed, we can filter out only them:

      removals.map do |change, path|
        change_without_file_children = change.dup
        change_without_file_children["children"] =
          (change_without_file_children["children"] || []).select do |child_change|
            %w[PBXGroup PBXVariantGroup].include?(child_change["isa"])
          end
        [change_without_file_children, path]
      end.each do |change, path|
        next unless %w[PBXGroup PBXVariantGroup].include?(change["isa"])

        group_path = join_path(path, change["displayName"])
        remove_component(project[group_path], change)
      end

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed as discussed by sorting

next unless %w[PBXGroup PBXVariantGroup].include?(change["isa"])

group_path = join_path(path, change["displayName"])

remove_component(project[group_path], change)
# by now we've deleted all of this group's children in the project, so we need to adapt the
# change to the expected current state of the group.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

change to the expected current state of the group, that is, without any children

@byohay byohay self-assigned this Sep 8, 2022
@ashdnazg ashdnazg force-pushed the feature/fix-group-moves branch 2 times, most recently from 8a230dc to 08846a1 Compare September 12, 2022 08:30
@ashdnazg
Copy link
Member Author

F&Sed

@byohay byohay merged commit fdc1d3c into main Sep 12, 2022
@byohay byohay deleted the feature/fix-group-moves branch September 12, 2022 11:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants