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.

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

Fix: Pick the meta object from actual resource rather from cached res… #783

Merged
merged 6 commits into from
Oct 30, 2022
Merged

Fix: Pick the meta object from actual resource rather from cached res… #783

merged 6 commits into from
Oct 30, 2022

Conversation

mountainfirefly
Copy link
Contributor

…ource if it is different

@mountainfirefly mountainfirefly mentioned this pull request Oct 28, 2022
Closed
@wopian
Copy link
Owner

wopian commented Oct 28, 2022 •
edited
Loading

Please can you fix the linter errors and warnings in the PR. You can run yarn lint --fix and it'll do most of it for you 👍

You can ignore the complexity warning flagged by Codeclimate

@wopian wopian linked an issue Oct 28, 2022 that may be closed by this pull request
Issue in Deserialize function: meta object is getting overwritten if the same reference is used multiple places. #784
Closed
@wopian wopian self-requested a review October 28, 2022 08:09
wopian
wopian requested changes Oct 28, 2022
View reviewed changes
packages/kitsu-core/src/linkRelationships/index.js Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
@mountainfirefly
Copy link
Contributor Author

Sure, will resolve these and update you soon.
Thanks

@mountainfirefly
Copy link
Contributor Author

@wopian I have addressed your review comments.
Please review again and let me know if anything else is needed.

wopian
wopian requested changes Oct 28, 2022
View reviewed changes
Copy link
Owner

@wopian wopian left a comment

Choose a reason for hiding this comment

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

Please run yarn lint --fix to resolve the lining errors and warnings from ESLint

packages/kitsu-core/src/deepEqual/index.js Outdated Show resolved Hide resolved
wopian
wopian requested changes Oct 28, 2022
View reviewed changes
packages/kitsu-core/src/deepEqual/index.js Outdated
Comment on lines 20 to 21
const objKeys1 = Object.keys(object1);
const objKeys2 = Object.keys(object2);
Copy link
Owner

Choose a reason for hiding this comment

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

Function should check both values are objects before contining to access Object.keys on them.

Tests are failing due to undefined and null values being passed in through object1 and/or object2.

@mountainfirefly
Copy link
Contributor Author

@wopian Your review comments have been addressed, please check now.

Thanks

@codeclimate
Copy link

codeclimate bot commented Oct 30, 2022

Code Climate has analyzed commit a66f183 and detected 0 issues on this pull request.

The test coverage on the diff in this pull request is 100.0% (100% is the threshold).

This pull request will bring the total coverage in the repository to 100.0% (0.0% change).

View more on Code Climate.

@wopian wopian merged commit 97ba151 into wopian:master Oct 30, 2022
@wopian
Copy link
Owner

wopian commented Oct 30, 2022

Thank you. I updated the tests to include coverage of the uncovered branches and added the sample you gave in the issue as a test case in linkedRelationships to test the behaviour works as intended.

@wopian
Copy link
Owner

wopian commented Oct 30, 2022

Released in 10.0.3 (2022-10-30)

@mountainfirefly
Copy link
Contributor Author

Thank you so much @wopian .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wopian wopian wopian approved these changes

Assignees

@mountainfirefly mountainfirefly

Labels
bug kitsu-core
Projects
None yet
Milestone
No milestone
2 participants
@mountainfirefly @wopian