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 bounded sequence exceeding bounds #21

Conversation

countvajhula
Copy link
Contributor

Hello!
I came across this bug:

sequence.rkt> (fifth (take 3 (naturals)))
4

sequence.rkt> (sequence->list (take 5 (take 3 (naturals))))
'(0 1 2 3 4)

sequence.rkt> (nth (take 3 (naturals)) 23)
23

Looks like a method name for the gen:countable implementation was wrong, fixed that and added tests. Here's the current output for the above:

sequence.rkt> (fifth (take 3 (naturals)))
; nth: index is out of range
;   index: 4
;   valid range: [0, 2]
;   sequence: #<lazy-sequence>
; Context:
;  .../collection/collection.rkt:81:0 body of "/Users/siddhartha/work/lisp/racket/racket-collections/collections-lib/data/collection/sequence.rkt"
;  .../racket/repl.rkt:11:26



sequence.rkt> (sequence->list (take 5 (take 3 (naturals))))
; take: length index is out of range
;   length index: 5
;   valid range: [0, 3]
;   sequence: #<lazy-sequence>
; Context:
;  /Users/siddhartha/work/lisp/racket/racket-collections/collections-lib/data/collection/sequence.rkt:263:0 body of "/Users/siddhartha/work/lisp/racket/racket-collections/collections-lib/data/collection/sequence.rkt"
;  .../racket/repl.rkt:11:26


sequence.rkt> (nth (take 3 (naturals)) 23)
; nth: index is out of range
;   index: 23
;   valid range: [0, 2]
;   sequence: #<lazy-sequence>
; Context:
;  .../collection/collection.rkt:81:0 body of "/Users/siddhartha/work/lisp/racket/racket-collections/collections-lib/data/collection/sequence.rkt"
;  .../racket/repl.rkt:11:26

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.09%) to 89.512% when pulling 02be753 on countvajhula:fix-bounded-sequence-exceeding-bounds into 5b3ec9b on lexi-lambda:master.

@countvajhula
Copy link
Contributor Author

Related to this PR, it looks like (length (take 5 (range 3))) ;=> 5. I haven't really looked into it but it might be because (known-finite? (range 3)) ;=> #f. Maybe the built-in in-range that's exported here should be wrapped in a struct that implements gen:countable. wdyt @lexi-lambda ?

@lexi-lambda lexi-lambda merged commit 7f498a1 into lexi-lambda:master Aug 16, 2020
@lexi-lambda
Copy link
Owner

Thanks for the fix, and sorry for taking so long to merge this. I’m not really maintaining this package, but I’ll try to stay more on top of any PRs.

@countvajhula
Copy link
Contributor Author

@lexi-lambda All good, thanks! Looks like the merge broke the build on master for some reason. I'll take a look soon if you don't get to it first.

@countvajhula
Copy link
Contributor Author

@lexi-lambda Re: the build failures on master, it looks like it's because of this change in an upstream cover-coveralls dependency. I think what's happening is that the cover package lists version exceptions for older versions of Racket, while the cover-lib package does not. So when Travis runs this command:

raco pkg install --auto cover cover-coveralls

... raco first installs the correct older version of cover for the version of Racket being tested, but is then is told to install the latest version as part of the dependencies for cover-coveralls, since that depends on cover-lib which does not indicate version exceptions.

I tried simply removing cover from this Travis step, but that doesn't work since the cover library uses some new code not present in older versions of Racket (in particular, the error in that Travis output indicates changes in this function).

Not sure what the right thing to do here is, I suppose cover-lib ought to include version exceptions for older versions of Racket for consistency with cover? Otherwise, as a stopgap, maybe cover-coveralls could go back to depending on cover as it's a more robust dependency? Or is there something that could be done here that doesn't require upstream changes? Seems like an upstream issue to me.

This pull request was closed.
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.

3 participants