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 range integer parsing #6897

Merged
merged 3 commits into from Nov 23, 2016

Conversation

6 participants
@FREEZX

FREEZX commented Nov 19, 2016

Fixes #6896

@mention-bot

This comment has been minimized.

mention-bot commented Nov 19, 2016

@FREEZX, thanks for your PR! By analyzing the history of the files in this pull request, we identified @janmeier, @onzag and @mickhansen to be potential reviewers.

@codecov-io

This comment has been minimized.

codecov-io commented Nov 19, 2016

Current coverage is 93.88% (diff: 100%)

Merging #6897 into v3 will not change coverage

Powered by Codecov. Last update d5caff2...b80316c

@onzag

This comment has been minimized.

Contributor

onzag commented Nov 19, 2016

@mickhansen I see that it's a very small diff, seems that it will cause no problem is right, but I'm not sure what happens there nor why I was chosen as reviewer; nor how the oid changed and it's only noticed until now.

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 19, 2016

Needs a PR to master first (btw v4 is gonna be out soon)

@FREEZX

This comment has been minimized.

FREEZX commented Nov 19, 2016

@felixfbecker Seems like its fixed in master ##6220, and i don't intend to switch to v4 right now .

@felixfbecker

This comment has been minimized.

Contributor

felixfbecker commented Nov 19, 2016

Then please do a direct backport of that fix (including tests)

@FREEZX

This comment has been minimized.

FREEZX commented Nov 19, 2016

Added the test from the fix, things seemed a bit messed up on my version of postgresql, so i had to explicitly set the inclusive values when creating the model instance. On the original in master, postgresql inserted [2,4) instead of (1, 4), even though the original query was doing

INSERT INTO "Ms" ("id","interval","createdAt","updatedAt") VALUES (DEFAULT,'(1,4)','2016-11-19 19:12:23.186 +00:00','2016-11-19 19:12:23.186 +00:00') RETURNING *;
return Model.findAll();
})
.spread(function(m){
console.log(m.interval);

This comment has been minimized.

@janmeier

janmeier Nov 23, 2016

Member

Remove

.spread(function(m){
console.log(m.interval);
expect(m.interval[0]).to.be.eql(1);
expect(m.interval[1]).to.be.eql(4);

This comment has been minimized.

@janmeier

janmeier Nov 23, 2016

Member

Please add an assertion for inclusive

@janmeier janmeier merged commit cf232e7 into sequelize:v3 Nov 23, 2016

4 checks passed

codecov/patch Coverage not affected when comparing d5caff2...b80316c
Details
codecov/project 93.88% (+0.00%) compared to d5caff2
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

janmeier added a commit that referenced this pull request Nov 23, 2016

@janmeier

This comment has been minimized.

Member

janmeier commented Nov 23, 2016

Thanks - Seems it was not properly fixed on master, I don't remember why there are two oids for each range though ...

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