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

Fixed class Object assign. #31

Merged
merged 5 commits into from
Oct 27, 2021
Merged

Fixed class Object assign. #31

merged 5 commits into from
Oct 27, 2021

Conversation

tripodsgames
Copy link
Contributor

@tripodsgames tripodsgames commented Sep 21, 2021

When we clone a class that uses Object.assign the cloning result is different. This PR fixes it by removing the hasOwnProperty check. This PR fixes it by checking if the property exists in the original object.

See #30 (comment) and #30 (comment)

Removed unnecessary hasOwnProperty.
@codecov-commenter
Copy link

codecov-commenter commented Sep 21, 2021

Codecov Report

Merging #31 (fd9cab1) into master (28565c3) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master       #31   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            4         4           
  Lines          115       115           
=========================================
  Hits           115       115           
Impacted Files Coverage Δ
src/index.js 100.00% <100.00%> (ø)
src/lite.js 100.00% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 28565c3...fd9cab1. Read the comment docs.

@lukeed
Copy link
Owner

lukeed commented Sep 21, 2021

Hi, please remove all code style changes. I can look at the diff after irrelevant changes have been removed.

@tripodsgames
Copy link
Contributor Author

Hi, please remove all code style changes. I can look at the diff after irrelevant changes have been removed.

Done it.

src/index.js Show resolved Hide resolved
@tripodsgames
Copy link
Contributor Author

@lukeed, if you want I can do the tests using Object.keys

@lukeed
Copy link
Owner

lukeed commented Oct 1, 2021

No thank you.

I'll review this over the weekend, sorry for the delay. Waiting because I'm pretty sure there was a reason tmp was used, but clearly I forgot to document that reason in the tests 🙈

@tripodsgames
Copy link
Contributor Author

No thank you.

I'll review this over the weekend, sorry for the delay. Waiting because I'm pretty sure there was a reason tmp was used, but clearly I forgot to document that reason in the tests see_no_evil

Ok, hope you can find a solution for this one, using the x instead of tmp fix the problem for me at least... by the way, i created a new test for Object.assign in this one. Use it in your tests to fix the Object.assign please, i use it a lot.

@tripodsgames
Copy link
Contributor Author

Any update?

@danieljackins
Copy link

Would also love for this PR to get merged 🙏

test/suites/class.js Outdated Show resolved Hide resolved
Copy link
Owner

@lukeed lukeed left a comment

Choose a reason for hiding this comment

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

Finally caught a second to review this & verify that it works as expected.

Thank you for the PR and for your patience on this 😃 Much appreciated!

@lukeed lukeed merged commit 7650274 into lukeed:master Oct 27, 2021
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.

4 participants