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.

Sign up for GitHub

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

Jump to bottom

Kintsugi: Avoid adding duplicates if settings don't allow. #76

Merged

byohay merged 2 commits into Lightricks:main from byohay:feature/avoid-adding-duplicates

Aug 4, 2022

Collaborator

byohay commented Jul 18, 2022

Kintusgi: Add Settings class.
Kintsugi: Avoid adding duplicates if settings don't allow.

byohay assigned ashdnazg

byohay force-pushed the feature/avoid-adding-duplicates branch 2 times, most recently from cd6cb75 to e2b25c8 Compare

July 18, 2022 11:38

ashdnazg requested changes

View reviewed changes

lib/kintsugi/apply_change_to_project.rb Outdated

-                    (old_value || []) + (added_change || []) - (removed_change || [])
+                    new_value = (old_value || []) + (added_change || []) - (removed_change || [])
+                    Settings.allow_duplicates ? new_value : new_value.uniq

Member

ashdnazg Aug 3, 2022

If you use uniq here, you will also delete duplicates in the old values, which is beyond the mandate given to Kintsugi.

lib/kintsugi/apply_change_to_project.rb Outdated

@@ @@ -506,6 +514,11 @@ def add_build_file(build_phase, change, change_path) @@
                       return
                     end
+                    duplicate_build_file_exist = build_phase.files.find do |build_file|

Member

ashdnazg Aug 3, 2022

Suggested change

      
                  duplicate_build_file_exist = build_phase.files.find do |build_file|
          
                  duplicate_build_file_exists = build_phase.files.find do |build_file|

lib/kintsugi/apply_change_to_project.rb Outdated

                   end
                   def add_subproject_reference(root_object, project_reference_change, change_path)
+                    duplicate_subproject_exist =

Member

ashdnazg Aug 3, 2022

Suggested change

      
                  duplicate_subproject_exist =
          
                  duplicate_subproject_exists =

lib/kintsugi/apply_change_to_project.rb Outdated

                         return
                       end
+                      return if !Settings.allow_duplicates &&

Member

ashdnazg Aug 3, 2022

also merge duplicate groups

byohay marked this pull request as ready for review

August 3, 2022 13:12

byohay force-pushed the feature/avoid-adding-duplicates branch from e2b25c8 to 4db7f04 Compare

August 3, 2022 13:12

Collaborator Author

byohay commented Aug 3, 2022

@ashdnazg round done.

byohay changed the title ~~feature/avoid adding duplicates~~ Kintsugi: Avoid adding duplicates if settings don't allow.

byohay linked an issue

that may be closed by this pull request

Support for ignoring duplicates #6

Closed

ashdnazg approved these changes

View reviewed changes

Member

ashdnazg left a comment

F&S&M

lib/kintsugi/apply_change_to_project.rb Outdated

+                  def find_file_in_group(group, filetype, filepath)
+                    group
+                      .children
+                      .select { |child| child.instance_of?(filetype) }

Member

ashdnazg Aug 4, 2022

why instance_of and not is_a?

Collaborator Author

byohay Aug 4, 2022

Because PBXVariantGroup inherit from PBXGroup, and I didn't want to treat them as duplicates if they have a different type.

spec/kintsugi_apply_change_to_project_spec.rb Outdated

@@ @@ -1400,6 +1380,164 @@ @@
                   expect(base_project).to be_equivalent_to_project(theirs_project)
                 end
+                describe "avoiding duplicate references to the same component" do
+                  it "avoids adding file reference that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding file reference that already exist" do
          
                it "avoids adding file reference that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(other_project).to be_equivalent_to_project(base_project)
+                  end
+                  it "avoids adding group that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding group that already exist" do
          
                it "avoids adding group that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(other_project).to be_equivalent_to_project(base_project)
+                  end
+                  it "avoids adding variant group that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding variant group that already exist" do
          
                it "avoids adding variant group that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(other_project).to be_equivalent_to_project(base_project)
+                  end
+                  it "avoids adding subproject that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding subproject that already exist" do
          
                it "avoids adding subproject that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(ours_project.root_object.project_references.count).to equal(1)
+                  end
+                  it "avoids adding build file that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding build file that already exist" do
          
                it "avoids adding build file that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(other_project).to be_equivalent_to_project(base_project)
+                  end
+                  it "avoids adding reference proxy that already exist" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "avoids adding reference proxy that already exist" do
          
                it "avoids adding reference proxy that already exists" do

spec/kintsugi_apply_change_to_project_spec.rb Outdated

+                    expect(other_project).to be_equivalent_to_project(base_project)
+                  end
+                  it "keeps array if adding string value that already exist in array" do

Member

ashdnazg Aug 4, 2022

Suggested change

      
                it "keeps array if adding string value that already exist in array" do
          
                it "keeps array if adding string value that already exists in array" do

byohay force-pushed the feature/avoid-adding-duplicates branch 5 times, most recently from e7a71a9 to 0bc1680 Compare

August 4, 2022 13:03

ashdnazg approved these changes

View reviewed changes

Member

ashdnazg left a comment

F&S&M

lib/kintsugi/apply_change_to_project.rb Outdated

Comment on lines 668 to 672

+                    duplicate_build_file_exists = !build_phase.files.find do |build_file|
+                      build_file.file_ref.path == change["fileRef"]["path"]
+                    end.nil?
+                    return if !Settings.allow_duplicates && duplicate_build_file_exists

Member

ashdnazg Aug 4, 2022

it's a bit hard to read when the ! is in the beginning and the .nil? is in the end.
consider

Suggested change

      
                  duplicate_build_file_exists = !build_phase.files.find do |build_file|
          
                    build_file.file_ref.path == change["fileRef"]["path"]
          
                  end.nil?
          
                  return if !Settings.allow_duplicates && duplicate_build_file_exists
          
                  duplicate_build_file = build_phase.files.find do |build_file|
          
                    build_file.file_ref.path == change["fileRef"]["path"]
          
                  end
          
                  return if !Settings.allow_duplicates && !duplicate_build_file.nil?

same in other places

Member

ashdnazg Aug 4, 2022

maybe existing_build_file instead of duplicate_build_file

lib/kintsugi/settings.rb Outdated

+              # frozen_string_literal: true
+              module Kintsugi
+                # Kintsugi global settings

Member

ashdnazg Aug 4, 2022

Suggested change

      
              # Kintsugi global settings
          
              # Kintsugi global settings.

byohay force-pushed the feature/avoid-adding-duplicates branch from 0bc1680 to a58f65b Compare

August 4, 2022 14:19


          Kintusgi: Add Settings class.

97d1813

The class contains global build settings, e.g. the `allow_duplicates`
flag.

byohay force-pushed the feature/avoid-adding-duplicates branch from a58f65b to c978bff Compare

August 4, 2022 14:35


          Kintsugi: Avoid adding duplicates if settings don't allow.

ed85ff5

This setting is relevant to:
- File references.
- Subprojects
- Build files
- Array build settings.
- Groups and variant groups.

byohay force-pushed the feature/avoid-adding-duplicates branch from c978bff to ed85ff5 Compare

August 4, 2022 14:36

byohay merged commit 123192e into Lightricks:main

byohay deleted the feature/avoid-adding-duplicates branch

August 4, 2022 14:37

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment