这是indexloc提供的服务，不要输入任何密码
Skip to content

Add AssignDeep function (fixes #90) #91

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
Closed
wants to merge 3 commits into from
Closed

Add AssignDeep function (fixes #90) #91

wants to merge 3 commits into from

Conversation

@mesqueeb
Copy link
Contributor

@mesqueeb mesqueeb commented Feb 12, 2020

🎁 Pull Request

  • Used a clear / meaningful title for this pull request
  • Tested the changes in your own code (on your projects)
  • Added / Edited tests to reflect changes (tst folder)
  • Have read the Contributing part of the Readme
  • Passed npm test

Fixes

fixes #90

Why have you made changes?

Object.Assign wasn't using deep merge

What changes have you made?

  • Added new AssignDeep to use a deep merge

What tests have you updated?

haven't updated the tests yet

Is there any breaking changes?

  • Yes, I changed the public API & documented it
  • Yes, I changed existing tests
  • No, I added to the public API & documented it
  • No, I added to the existing tests
  • I don't know

Anything else worth mentioning?

The PR process is a bit overwhelming and I just needed something up and running fast, that's why I made it on my fork.

This PR was made just in case, but feel free to close it because I didn't add tests. 😉

@mesqueeb mesqueeb requested a review from millsp as a code owner February 12, 2020 07:39
millsp
millsp requested changes Feb 12, 2020
View reviewed changes
Copy link
Owner

@millsp millsp left a comment

Choose a reason for hiding this comment

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

Hi @mesqueeb

I need a few thing for pulling this:

  • The tests must pass. This is a heavy type so it's possible it does not.
  • Your changes must go into the Assign.ts file.
  • Add a deep option on Assign, like for Merge

Cheers

@mesqueeb mesqueeb closed this Feb 12, 2020
@mesqueeb mesqueeb reopened this Feb 12, 2020
@mesqueeb mesqueeb closed this Feb 12, 2020
@mesqueeb
Copy link
Contributor Author

mesqueeb commented Feb 13, 2020

@pirix-gh
these are the errors I get on npm test
image

@mesqueeb mesqueeb reopened this Feb 13, 2020
@mesqueeb mesqueeb mentioned this pull request Feb 13, 2020
Closed
@mesqueeb
Copy link
Contributor Author

mesqueeb commented Feb 13, 2020

@pirix-gh I did:

  • Add a deep option on Assign, like for Merge

and I did this in Assign.ts, like you asked.

The tests don't work, but it's a problem with Object, not with my PR.
I thank you very much for your consideration!!
🎉

@millsp millsp closed this Feb 13, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@millsp millsp millsp requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Use Object.Merge with unlimited arguments

2 participants

@mesqueeb @millsp