fix: still show ranges if there are multiple ranges, even if some are invalid #63
Conversation
bjohansebas
changed the title
| it('should return -2 for range missing dash', function () { | ||
| assert.strictEqual(parse(200, 'bytes=100200'), -2) | ||
| assert.strictEqual(parse(200, 'bytes=,100200'), -2) | ||
| assert.strictEqual(parse(200, 'bytes=100-200,100200'), -2) | ||
| }) |
There was a problem hiding this comment.
This is to get the coverage to 100%, it has nothing to do with my changes
bjohansebas
force-pushed
the
more-spec
branch
from
335ad3f to
7622b94
Compare
bjohansebas
force-pushed
the
more-spec
branch
from
1ec15b7 to
c0bb7ea
Compare
index.js
Outdated
| // ignore empty/whitespace-only ranges if there are other valid ranges | ||
| if (arr.length > 1 && arr[i].trim() === '') { | ||
| continue | ||
| } |
There was a problem hiding this comment.
This handles a case where the range can be 'bytes= , 0-10', which before the two PRs was valid and returned ranges, but without this patch it returns -2.
|
another case that used to work before the previous two PRs, but no longer does now var range = parse(1000, 'bytes= d ,20-30')
assert.strictEqual(range.type, 'bytes')
assert.strictEqual(range.length, 1)
deepEqual(range[0], { start: 20, end: 30 }) |
|
I'm going to edit this PR, it's a bit too complex and contains edge cases. Any time you're trying to check lengths of things and have a bunch of branches like this it's going to contain subtle problems, it's a lot easier to rely on the parser that's already written. It's why I got rid of the length checks in #58 (it had bugs for whitespace and other characters). The simplest approach is to just keep a return code value and alter it during parsing results, then return it as the end. Finally, I think back porting the code from #62 should be the final outcome here, since it doubled performance instead of reducing it, while also improving correctness. |
test/range-parser.js
Outdated
| it('should return -2 for range missing dash', function () { | ||
| assert.strictEqual(parse(200, 'bytes=100200'), -2) | ||
| assert.strictEqual(parse(200, 'bytes=,100200'), -2) | ||
| assert.strictEqual(parse(200, 'bytes=100-200,100200'), -2) |
There was a problem hiding this comment.
Once I removed the return this failed but arguably should be working, 100-200 is valid.
test/range-parser.js
Outdated
| assert.strictEqual(parse(200, 'bytes=500-600,x-'), -1) | ||
| assert.strictEqual(parse(200, 'bytes=x-,500-600'), -1) | ||
| assert.strictEqual(parse(200, 'bytes=500-600,900-'), -1) | ||
| assert.strictEqual(parse(200, 'bytes=900-,500-600'), -1) |
There was a problem hiding this comment.
Ditto for this test, 500-600 is valid. I think this test is just invalid now.
There was a problem hiding this comment.
Isn’t it supposed to be out of range? And is that why it’s -1?
There was a problem hiding this comment.
It is a -1 this is already being tested above.
There was a problem hiding this comment.
Your PR previously had it as Misremembered, my edit changes it to a -2.-2. I can go either way I guess, but I'd err to simplicity.
There was a problem hiding this comment.
Added the test back, do you need invalid and unsatisfiable to return -1?
There was a problem hiding this comment.
I think it should still return -1. There’s a valid range, but it exceeds the allowed bounds, so it should return -1
2 participants
Before #57 and in #58, if there were multiple ranges and some of them were valid, they would still be shown. at least this way those changes aren’t a completely breaking change.