apply_change_to_project: use attribute_name for building change_path. #119
Conversation
| @@ -1022,6 +1020,7 @@ def add_attributes_to_component(component, change, change_path, ignore_keys: []) | |||
| next if (%w[isa displayName] + ignore_keys).include?(change_name) | |||
|
|
|||
| attribute_name = attribute_name_from_change_name(change_name) | |||
| attribute_path = "#{change_path}/#{change_name}" | |||
| @@ -1022,6 +1020,7 @@ def add_attributes_to_component(component, change, change_path, ignore_keys: []) | |||
| next if (%w[isa displayName] + ignore_keys).include?(change_name) | |||
|
|
|||
| attribute_name = attribute_name_from_change_name(change_name) | |||
| attribute_path = "#{change_path}/#{change_name}" | |||
There was a problem hiding this comment.
Shouldn't you use attribute_name instead of change_name?
If not, let's call it child_path, wdyt?
There was a problem hiding this comment.
I don't want to use the attribute name here since it's being converted later in child_component when traversing the tree in component_at_path.
| @@ -306,7 +306,8 @@ def apply_change_to_component(parent_component, change_name, change, parent_chan | |||
| component = child_component(parent_component, change_name) | |||
| elsif change[:added].is_a?(Array) | |||
| (change[:added]).each do |added_change| | |||
| add_child_to_component(parent_component, added_change, change_path) | |||
| add_child_to_component(parent_component, added_change, | |||
| "#{change_path}/#{added_change["displayName"]}") | |||
ashdnazg
force-pushed
the
feature/fix-paths
branch
2 times, most recently
from
e9b80bd to
550b9b6
Compare
|
F&Sed |
| @@ -1022,6 +1020,7 @@ def add_attributes_to_component(component, change, change_path, ignore_keys: []) | |||
| next if (%w[isa displayName] + ignore_keys).include?(change_name) | |||
|
|
|||
| attribute_name = attribute_name_from_change_name(change_name) | |||
| child_path = join_path(change_path, attribute_name) | |||
There was a problem hiding this comment.
I'm confused, you said you don't want to use attribute_name here, no?
Then again, if you don't use attribute_name here I don't understand what this PR fixes
There was a problem hiding this comment.
you're right, I changed it too many times and got confused 🤦
There was a problem hiding this comment.
This PR fixes the issue by not using the display name. It doesn't matter if you use the child name (camel case) or the attribute name (snake case), both are correct and converted later to snake case when the path is used.
| @@ -306,7 +306,8 @@ def apply_change_to_component(parent_component, change_name, change, parent_chan | |||
| component = child_component(parent_component, change_name) | |||
There was a problem hiding this comment.
Don't you need to change line 305 as well?
There was a problem hiding this comment.
No, we only need to use the display names when adding array elements. change_path already includes the key under which the hash is.
When merging the tree, Kintsugi keeps track of where it is, to allow finding the corresponding component in the source project. Until this commit, the path was constructed in `add_child_to_component` by appending the display name of the added component. When adding a file reference this path was used to traverse the project tree along each object's attributes. This caused a bug when the display name and the attribute name are different, e.g., a target's build configuration list has a display name of "ConfigurationList" and an attribute name of "build_configuration_list". The camel-case was converted correctly, but the merge failed due to not finding the attribute "configuration_list". The solution to this issue is to use the attribute names to create the path, except when adding an object to a list, where there is no attribute and then the display name is used.
ashdnazg
force-pushed
the
feature/fix-paths
branch
from
550b9b6 to
4c26a59
Compare
|
F&Sed |
Fixes #118
When merging the tree, Kintsugi keeps track of where it is, to allow
finding the corresponding component in the source project.
Until this commit, the path was constructed in
add_child_to_componentby appending the display name of the added component.
When adding a file reference this path was used to traverse the project
tree along each object's attributes.
This caused a bug when the display name and the attribute name
are different, e.g., a target's build configuration list has a display
name of "ConfigurationList" and an attribute name of
"build_configuration_list". The camel-case was converted correctly, but
the merge failed due to not finding the attribute "configuration_list".
The solution to this issue is to use the attribute names to create the
path, except when adding an object to a list, where there is no
attribute and then the display name is used.