Skip to content

Conversation

guzman-raphael
Copy link
Collaborator

@guzman-raphael guzman-raphael commented Apr 15, 2020

Fix #211
also:

  • Fixes bug with createSchema on MySQL8
  • Updates unit tests to new structure
  • Add doc on running unit tests locally

Copy link
Contributor

@eywalker eywalker left a comment

Choose a reason for hiding this comment

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

I think we have to only accept [] for null value and not empty string ''

+dj/Relvar.m Outdated
valueStr = 'NULL';
value = {};
case header.attributes(ix).isString
assert(dj.lib.isString(value), 'Value must be a string')
Copy link
Contributor

Choose a reason for hiding this comment

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

Under this code, it appears that passing value of an empty string '' will make it interpreted as NULL but that's not what we'd like. We only want to accept [] as a value for empty/NULL for non numeric types. This assertion will fail for [] and also unfortunately isemtpy(value) is true for value of ''

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

+dj/Relvar.m Outdated
valueStr = 'NULL';
value = {};
case header.attributes(ix).isString
assert(dj.lib.isString(value), 'Value must be a string')
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I don't think this will allow for inserting [] as isString([]) would be false.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Addressed

@guzman-raphael guzman-raphael requested a review from eywalker April 22, 2020 20:47
valueStr = '"{M}"';
value = {value};
end
valueStr = '"{M}"';
Copy link
Member

Choose a reason for hiding this comment

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

how is NULL handled here?

Copy link
Member

Choose a reason for hiding this comment

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

oh, I see, it's already handled together for all attribute types.

assert(~header.attributes(ix).iskey, 'cannot update a key value. Use insert(..,''REPLACE'') instead')
assert(numel(ix)==1, 'invalid attribute name');
assert(~header.attributes(ix).iskey, ...
'cannot update a key value. Use insert(..,''REPLACE'') instead');
Copy link
Contributor

Choose a reason for hiding this comment

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

beautifully handled

@eywalker eywalker merged commit 853e2d2 into master Apr 22, 2020
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.

update back to null does not work for date or datetime

3 participants