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

replace lodash.,merge with object.assign #884

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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