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

Fix store null values in lists #424

Merged
merged 5 commits into from Dec 27, 2018

Conversation

Projects
None yet
3 participants
@T4rk1n
Copy link
Contributor

commented Dec 26, 2018

Assert that both old and new data is not null when checking if the data needs update.

Part of fix for #422.

@rmarren1 please review.

T4rk1n added some commits Dec 26, 2018

@T4rk1n T4rk1n requested a review from rmarren1 Dec 26, 2018

@rmarren1

This comment has been minimized.

Copy link
Contributor

commented Dec 27, 2018

💃

if (R.isNil(old) || R.isNil(data)) {
const oldNull = R.isNil(old);
const newNull = R.isNil(data);
if ((oldNull || newNull) && !(oldNull && newNull)) {

This comment has been minimized.

Copy link
@alexcjohnson

alexcjohnson Dec 27, 2018

Contributor

I found this a bit hard to read... how about if (oldNull !== newNull) ? Or I suppose you could short-circuit the logic below with something like if (oldNull || newNull) { return oldNull !== newNull; }

Separately, it looks to me as though the checks below implicitly assume old and data have the same type, and may fail if for example data is an Array and old is an Object. Should we include a step if (R.type(old) !== type) { return false; }?

This comment has been minimized.

Copy link
@T4rk1n

T4rk1n Dec 27, 2018

Author Contributor

Separately, it looks to me as though the checks below implicitly assume old and data

That's the second part of #422, I'll add a test and fix it in another PR. Initial implementation assumed data was of the same shape.

// Assuming data and old are of the same type.

@T4rk1n T4rk1n merged commit 51f04a3 into master Dec 27, 2018

4 checks passed

ci/circleci: python-2.7 Your tests passed on CircleCI!
Details
ci/circleci: python-3.6 Your tests passed on CircleCI!
Details
ci/circleci: python-3.7 Your tests passed on CircleCI!
Details
percy/dash-core-components Visual review automatically approved, no visual changes found.
Details

@T4rk1n T4rk1n deleted the fix-store-null branch Dec 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.