Skip to content

Conversation

@jimmywarting
Copy link

No description provided.

@kwakwaversal
Copy link

While the change set effectively removes lodash.merge as a dependency, the MR lacks accompanying documentation or a description to clarify the rationale behind this change.

It would be beneficial to understand the purpose of this change. Is the goal to reduce dependencies, improve performance, or something else?

Also, it's worth noting that lodash.merge and Object.assign are not like-for-like replacements. lodash.merge performs a deep merge, while Object.assign performs a shallow merge. However, upon review of the changed code snippet:

const schema = Object.assign(
  {},
  version === 2 ? openapi2Schema : openapi3Schema,
  args ? args.extensions : {}
);

it seems like a deep merge might not be necessary in this context. Therefore, using Object.assign appears to be appropriate for this specific use case.

Is it worth updating the MR description to include the rationale behind this change for the sake of clarity?

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.

2 participants