apply_change_to_project: Resolve some merge errors interactively. #96

byohay · 2023-02-14T13:16:34Z

With this feature, Kintsugi will ask for a guide whenever a conflict that it cannot solve on its own has happend. For example:

apply_change_to_project: Add change source project parameter.
apply_change_to_project: Set component after it was added.
Kintsugi tests: Add test for ignoring change to non existent component.
Modify display name of container item proxy
apply_change_to_project: Resolve some merge errors interactively.

The function can continue executing after the component is added.

ashdnazg

F&S

ashdnazg · 2023-03-14T13:45:53Z

lib/kintsugi/apply_change_to_project.rb

+        component = project.group_or_file_at_path(path)

        change.each do |subchange_name, subchange|
          next if subchange_name == "children"

+          if component.nil?
+            unless ConflictResolver.changing_nonexistent_component(path)
+              @ignored_components_group_paths.append(path)
+              break
+            end
+
+            @created_components_group_paths.append(path)
+            component = create_nonexistent_groupable_component(project, path)
+          end
+
          apply_change_to_component(component, subchange_name, subchange, path)


Suggested change

component = project.group_or_file_at_path(path)

change.each do |subchange_name, subchange|

next if subchange_name == "children"

if component.nil?

unless ConflictResolver.changing_nonexistent_component(path)

@ignored_components_group_paths.append(path)

break

end

@created_components_group_paths.append(path)

component = create_nonexistent_groupable_component(project, path)

end

apply_change_to_component(component, subchange_name, subchange, path)

component = project.group_or_file_at_path(path)

if component.nil? && !change.keys.to_set.subset?(["children"].to_set)

unless ConflictResolver.changing_nonexistent_component(path)

@ignored_components_group_paths.append(path)

next

end

@created_components_group_paths.append(path)

component = create_nonexistent_groupable_component(project, path)

end

change.each do |subchange_name, subchange|

next if subchange_name == "children"

apply_change_to_component(component, subchange_name, subchange, path)

note that the break needs to be next - the tests passed with break so maybe add a test for that if you're bored

Did you use !change.keys.to_set.subset?(["children"].to_set) to account for the case where keys is empty? Is this a possible scenario?
Anyway if that's the only reason I prefer !change.keys.empty? && change.keys != ["children"]

I don't know if it's possible - I tried to keep the logic identical to the previous situation.
Your proposal is fine by me

ashdnazg · 2023-03-14T13:55:46Z

lib/kintsugi/cli.rb

+        unless options[:"interactive-resolution"].nil?
+          Settings.interactive_resolution = options[:"interactive-resolution"]
+        end


Shouldn't this be according to whether a tty is available?

This is handled in Kintsugi::Settings.

ashdnazg · 2023-03-14T13:57:12Z

lib/kintsugi/conflict_resolver.rb

+require "tty-prompt"
+
+module Kintsugi
+  class ConflictResolver


It's a bit hard to know what these functions return - some are bool (maybe suffix them with ?), some are all kind of overrides that maybe should be reflected in the name.

In any case, these functions should be doc'd

The default display name is just a generic name for all `PBXContainerItemProxy`. Many functions rely on `display_name` to identify objects and making it identifiable is easier to program.

ashdnazg

F&S&M

lib/kintsugi/apply_change_to_project.rb

ashdnazg

F&S&M

ashdnazg · 2023-03-29T13:22:48Z

lib/kintsugi/cli.rb

@@ -183,7 +193,10 @@ def create_root_command
          exit
        end

-        opts.on("--allow-duplicates", "Allow to add duplicates of the same entity")
+        opts.on("--allow-duplicates-FLAG", "Allow to add duplicates of the same entity")


This doesn't seem to be on purpose

In order to resolve some of the merge errors, `theirs` project is added a parameter to `apply_change_to_project` to compensate for lost information about components that were removed from `project`. Some refactorings: - When checking if something is a PBXGroup using `is_a`, there's no need to check if it's a PBXVariantGroup because it inherits from PBXGroup.

byohay assigned ashdnazg Feb 14, 2023

byohay changed the title ~~feature/tty prompt resolve conflicts2~~ apply_change_to_project: Resolve some merge errors interactively. Feb 14, 2023

byohay linked an issue Feb 14, 2023 that may be closed by this pull request

Better support for real conflicts #5

Closed

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 2 times, most recently from 120f497 to 27e5002 Compare February 14, 2023 13:44

byohay closed this Feb 14, 2023

byohay deleted the feature/tty-prompt-resolve-conflicts2 branch February 14, 2023 13:44

byohay restored the feature/tty-prompt-resolve-conflicts2 branch February 14, 2023 13:44

byohay reopened this Feb 14, 2023

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 6 times, most recently from 0b782da to 809e236 Compare February 19, 2023 07:26

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 7 times, most recently from d9f34c1 to c43ff80 Compare February 23, 2023 16:21

byohay marked this pull request as ready for review February 23, 2023 16:24

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 4 times, most recently from 8ab4dfc to 754f6e2 Compare February 25, 2023 13:34

byohay added 2 commits March 5, 2023 16:12

apply_change_to_project: Set component after it was added.

1d50863

The function can continue executing after the component is added.

Kintsugi tests: Add test for ignoring change to non existent component.

f56bac0

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch from 754f6e2 to 4e1d3ed Compare March 5, 2023 14:22

ashdnazg approved these changes Mar 14, 2023

View reviewed changes

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch from 4e1d3ed to 2b7b7c7 Compare March 17, 2023 13:50

Xcodeproj: Extend display name of container item proxy.

13b40a8

The default display name is just a generic name for all `PBXContainerItemProxy`. Many functions rely on `display_name` to identify objects and making it identifiable is easier to program.

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 2 times, most recently from e6a5ca1 to 24c6aa5 Compare March 17, 2023 14:40

ashdnazg approved these changes Mar 21, 2023

View reviewed changes

lib/kintsugi/apply_change_to_project.rb Outdated Show resolved Hide resolved

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch 7 times, most recently from 8de713b to f41d9fa Compare March 29, 2023 13:10

ashdnazg approved these changes Mar 29, 2023

View reviewed changes

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch from f41d9fa to b00d28b Compare March 29, 2023 13:57

byohay force-pushed the feature/tty-prompt-resolve-conflicts2 branch from b00d28b to c51435d Compare March 29, 2023 14:02

byohay merged commit 9e845c4 into Lightricks:main Mar 29, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

apply_change_to_project: Resolve some merge errors interactively. #96

apply_change_to_project: Resolve some merge errors interactively. #96

byohay commented Feb 14, 2023 •

edited

Loading

ashdnazg left a comment

ashdnazg Mar 14, 2023

ashdnazg Mar 14, 2023

byohay Mar 14, 2023 •

edited

Loading

ashdnazg Mar 14, 2023

ashdnazg Mar 14, 2023

byohay Mar 14, 2023

ashdnazg Mar 14, 2023

ashdnazg left a comment

ashdnazg left a comment

ashdnazg Mar 29, 2023

byohay Mar 29, 2023

apply_change_to_project: Resolve some merge errors interactively. #96

apply_change_to_project: Resolve some merge errors interactively. #96

Conversation

byohay commented Feb 14, 2023 • edited Loading

ashdnazg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byohay Mar 14, 2023 • 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

ashdnazg left a comment

Choose a reason for hiding this comment

ashdnazg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

byohay commented Feb 14, 2023 •

edited

Loading

byohay Mar 14, 2023 •

edited

Loading