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: Handle remove configuration list change. #82

Merged

Conversation

byohay
Copy link
Collaborator

@byohay byohay commented Aug 9, 2022

Also, refactor applying added changes for symmetry with removed changes.

Copy link
Member

@ashdnazg ashdnazg left a comment

Choose a reason for hiding this comment

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

F&S

elsif change[:removed].is_a?(Array)
(change[:removed]).each do |removed_change|
child =
child_component_of_object_list(component, removed_change, removed_change["displayName"])
Copy link
Member

Choose a reason for hiding this comment

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

this function expects component to be non-nil

add_child_to_component(is_object_list ? parent_component : component, added_change,
change_path)
if change[:added].is_a?(Hash)
add_child_to_component(parent_component, change[:added], change_path)
Copy link
Member

Choose a reason for hiding this comment

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

same

Comment on lines -214 to -217
if component.nil?
add_missing_component_if_valid(parent_component, change_name, change, change_path)
return
end
Copy link
Member

Choose a reason for hiding this comment

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

The nullability questions arise from this

@byohay byohay force-pushed the feature/handle-remove-configuration-list branch from 688d288 to 34367c6 Compare August 11, 2022 06:09
@byohay
Copy link
Collaborator Author

byohay commented Aug 11, 2022

@ashdnazg I added guards around places that need component. I also removed this dependency in change[:added], as it always needs the parent component.

Copy link
Member

@ashdnazg ashdnazg left a comment

Choose a reason for hiding this comment

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

I think it's ok,
Note that in the commit message you still mention symmetry, but that's no longer the case.
Also we need to remember to fail added if it will override a different existing value. Probably in a different PR.

Also, refactor applying added changes for symmetry with removed changes.
@byohay byohay force-pushed the feature/handle-remove-configuration-list branch from 34367c6 to c40e77a Compare August 15, 2022 13:59
@byohay byohay merged commit cbad1fd into Lightricks:main Aug 15, 2022
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