RFC: Expand documentation of seek, position and related functions
#58024
Conversation
This commit intentionally omits expanding the documentation of `mark` etc., which although related, is a somewhat different mechanism.
jakobnissen
added
needs decision
jakobnissen
changed the title
seek, position` and related functionsseek, position and related functions
|
LGTM. My understanding is that BGZF implementation does not provide |
| The `pos` argument should be a value that can be returned by `position(s)`. | ||
| Seeking before the first position should throw an error. Seeking after the | ||
| final position should throw an error if the stream is not writable. | ||
| If the stream is writable, reading will throw an error, and writing will fill |
There was a problem hiding this comment.
It should be okay for read to return an empty vector instead of throwing an error. That is how IOStream, and GZipStream currently behave after seeking past the end.
| Seek a stream relative to the current position. | ||
| This is equivalent to `seek(s, position(s) + offset)`, except where seeking would | ||
| seek beyond the end of the stream, where `skip` will throw an `EOFError`. |
There was a problem hiding this comment.
I think it might be better to leave the case of skip beyond the end of the file as implementation-defined behavior.
Allowing skip to go beyond the end of the stream to match the behavior of IOStream in TranscodingStreams is possible, but I think it would require keeping track of a bunch of additional states like zlib does in https://github.com/madler/zlib/blob/5a82f71ed1dfc0bec044d9702463dbdf84ea3b71/gzguts.h#L188-L196, basically making skip lazy and then running any requested skips on every read. https://github.com/madler/zlib/blob/5a82f71ed1dfc0bec044d9702463dbdf84ea3b71/gzread.c#L276-L281
Co-authored-by: Jameson Nash <[email protected]>
The intention is to add just a minimal doc string. Other PRs attempt to document the interface, such as: * JuliaLang#41291 * JuliaLang#58024 * JuliaLang#59069
The intention is to add just a minimal doc string. Other PRs attempt to document the interface, such as: * #41291 * #58024 * #59069 --------- Co-authored-by: Jakob Nybo Nissen <[email protected]>
The intention is to add just a minimal doc string. Other PRs attempt to document the interface, such as: * JuliaLang#41291 * JuliaLang#58024 * JuliaLang#59069 --------- Co-authored-by: Jakob Nybo Nissen <[email protected]>
4 participants
This commit intentionally omits expanding the documentation of
marketc., which although related, is a somewhat different mechanism.I'm interested in discussion about what the right behaviour is:
Here, I documented
seekis able to seek past the end of the IO, if the IO is seekable, in which case writing to it will fill in zero bytes. But I don't actually like this. It feels like that API is mixing up the behaviour oftruncateandseek. It really feels like this should be an error. It also makes the behaviour ofskipmore awkward, because implementing this by reading a bunch of bytes no longer corresponds to seeking. It also creates a weird mismatch between reading and writing IOs. IMO, the congruence with libc'slseekis not enough to warrant this. IMO this should throw an exception when the seeking goes out of bounds in either direction.This PR kind of chickens out and doesn't go ahead and say that
positionshould return a zero-basedInt. Having it be more generic is nice for e.g. BGZF's virtualoffset, but it also makes it harder to use. Probably, IOs will not document what theirpositionreturns, so in practice, users will just rely on it returning a zero-basedIntwithout it being documented.A comment from Jameson suggests
position's return value should support arithmetic. That makes sense, since it then allowsskipand relative seeking. However, the only non-integer position type I know of, BGZF's virtual offset, does not support arithmetic. So it feels like documenting this gets the worst of both worlds: It neither enables custom position types, nor do you get the convenience of knowing the return value is a zero-indexedIntTODO
Closes #57618
Closes #57639