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

feat: add get_{ref, mut} to arrow_ipc Reader and Writer #4122

Merged
merged 1 commit into from
Apr 25, 2023

Conversation

sticnarf
Copy link
Contributor

Which issue does this PR close?

Closes #4121.

Rationale for this change

What changes are included in this PR?

The BufReader and BufWriter in Rust std have similar methods.

Are there any user-facing changes?

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf changed the title feat: add get_{ref, mut} to arrow_ipc::File{Reader, Writer} feat: add get_{ref, mut} to arrow_ipc Reader and Writer Apr 25, 2023
Copy link
Member

@waynexia waynexia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking great 👍 thanks @sticnarf for your contribution

Copy link
Contributor

@tustvold tustvold left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, my only concern is that get_mut might cause users to rely on implicit behaviour of the outer types (e.g. when they flush, etc...) but the "inadvisable" comment hopefully protects us against this


/// Gets a mutable reference to the underlying reader.
///
/// It is inadvisable to directly read from the underlying reader.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

Comment on lines +1043 to +1044
///
/// It is inadvisable to directly read from the underlying reader.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///
/// It is inadvisable to directly read from the underlying reader.

Reading requires mutable access so this comment is possibly redundant

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sometimes reading doesn't require a mutable reference (e.g. &File implements Read), so I think the comment is worth having.

Comment on lines +1262 to +1263
///
/// It is inadvisable to directly read from the underlying reader.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
///
/// It is inadvisable to directly read from the underlying reader.

@tustvold tustvold merged commit 244cd92 into apache:master Apr 25, 2023
@sticnarf sticnarf deleted the access-inner branch April 26, 2023 05:45
@tustvold tustvold added the arrow Changes to the arrow crate label May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
arrow Changes to the arrow crate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support accessing ipc Reader/Writer inner by reference
3 participants