Coerce options which should be numbers to number #4578

Closed
wheresrhys opened this Issue Nov 15, 2015 · 4 comments

Comments

Projects
None yet
2 participants
@wheresrhys
Contributor

wheresrhys commented Nov 15, 2015

e.g limit is ignored in the following

db.query(`${indexName}/index`, {
    limit: "4"
})

Either this should error/warn or coerce to a number (which would be nice as these values will often come from url query strings), but just getting ignored isn't particularly helpful.

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 16, 2015

Member

For a simple case like this, I think coercing them would be fine, cheers for the report, if you wanted to make PR for it would be very happy to help

Member

daleharvey commented Nov 16, 2015

For a simple case like this, I think coercing them would be fine, cheers for the report, if you wanted to make PR for it would be very happy to help

@wheresrhys

This comment has been minimized.

Show comment
Hide comment
@wheresrhys

wheresrhys Nov 17, 2015

Contributor

Great. I'll have a go some time in the next few days. If a value can't be
coerced to a number what are your thoughts on behaviour. I'd favour
erroring - particularly as its wrapped in the safety of a promise - but a
warning seems reasonable too
On Mon, 16 Nov 2015 at 9:11 p.m., Dale Harvey notifications@github.com
wrote:

For a simple case like this, I think coercing them would be fine, cheers
for the report, if you wanted to make PR for it would be very happy to help


Reply to this email directly or view it on GitHub
#4578 (comment).

Contributor

wheresrhys commented Nov 17, 2015

Great. I'll have a go some time in the next few days. If a value can't be
coerced to a number what are your thoughts on behaviour. I'd favour
erroring - particularly as its wrapped in the safety of a promise - but a
warning seems reasonable too
On Mon, 16 Nov 2015 at 9:11 p.m., Dale Harvey notifications@github.com
wrote:

For a simple case like this, I think coercing them would be fine, cheers
for the report, if you wanted to make PR for it would be very happy to help


Reply to this email directly or view it on GitHub
#4578 (comment).

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 20, 2015

Member

Yup if we are given a value and expecting a certain type then I would throw an error. One thing I would like to mention is that the check should be done as deep as possible to where it is actually used and not checking on the way in. I think that way has a lot of benefits

Member

daleharvey commented Nov 20, 2015

Yup if we are given a value and expecting a certain type then I would throw an error. One thing I would like to mention is that the check should be done as deep as possible to where it is actually used and not checking on the way in. I think that way has a lot of benefits

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 30, 2015

Member

Fixed in 112faf9

Member

daleharvey commented Nov 30, 2015

Fixed in 112faf9

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