Skip to content
This repository has been archived by the owner on Feb 18, 2024. It is now read-only.

FileReader doesn't own underlying reader? #514

Closed
blakesmith opened this issue Oct 9, 2021 · 2 comments · Fixed by #518
Closed

FileReader doesn't own underlying reader? #514

blakesmith opened this issue Oct 9, 2021 · 2 comments · Fixed by #518
Labels
enhancement An improvement to an existing feature no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog

Comments

@blakesmith
Copy link
Contributor

blakesmith commented Oct 9, 2021

Hi there!

Is there a reason FileReader doesn't own the underlying reader? StreamReader owns its underlying reader, and as of 0.6.0, FileWriter now owns its underlying writer as well.

I've been trying to work with FileReaders, and have been having trouble with it taking a &'a mut R instead of just R. This makes it hard if I want to store a FileReader in another struct to make multiple calls to the underlying iterator without fighting the borrow checker. For now, I'm using a StreamReader instead, with a file to work around the issue, but StreamReaders seem to have slightly different semantics.

Would it be better for these IO interfaces to consistently take ownership of their underlying readers and writers? Would ya'll accept a patch if so? Happy to post some more concrete examples if that would help!

Thanks for arrow2! It's really been a pleasure to work with so far!

@jorgecarleitao jorgecarleitao added the enhancement An improvement to an existing feature label Oct 10, 2021
@jorgecarleitao
Copy link
Owner

I agree. This was an early experiment and I admit I am not happy either. Let's add a into_inner like the std::io::Cursor has, to take ownership back and remove the lifetime.

Would you like to try it out?

Thanks for arrow2! It's really been a pleasure to work with so far!

Thanks! 🙇

@blakesmith
Copy link
Contributor Author

Thanks @jorgecarleitao! I took a crack at the change in #518, including your into_inner suggestion. I would love to hear your feedback!

@jorgecarleitao jorgecarleitao added the no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog label Oct 10, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement An improvement to an existing feature no-changelog Issues whose changes are covered by a PR and thus should not be shown in the changelog
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants