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

Improve IO#read, IO#readpartial, IO#write and IO.copy_stream #521

Merged
merged 1 commit into from
Dec 17, 2020
Merged

Improve IO#read, IO#readpartial, IO#write and IO.copy_stream #521

merged 1 commit into from
Dec 17, 2020

Conversation

This change aims to improve the following method signatures of the `IO` class:

- `#read`
- `#readpartial`
- `#write`
- `.copy_stream`
# need the behavior like a single read(2) system call, consider #readpartial,
# #read_nonblock, and #sysread.
#
def read: (?Integer? length, ?String outbuf) -> String?
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] Make the first argument type nilable because the API doc says:

length must be a non-negative integer or nil.

-def read: (?Integer length, ?String outbuf) -> String?
+def read: (?Integer? length, ?String outbuf) -> String?

# on the situation IO#sysread causes Errno::EWOULDBLOCK as if the fd is blocking
# mode.
#
def readpartial: (Integer maxlen, ?String outbuf) -> String
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] Simplify overloading.

-def readpartial: (Integer maxlen) -> String
-               | (Integer maxlen, ?String outbuf) -> String
+def readpartial: (Integer maxlen, ?String outbuf) -> String

# This is a test
# That was 15 bytes of data
#
def write: (*_ToS string) -> Integer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] Rename the argument (arg0 -> string) for readability according to the API doc.

# When *src_offset* is specified and *src* is an IO, IO.copy_stream doesn't move
# the current file offset.
#
def self.copy_stream: ((String | _Reader | _ReaderPartial) src, (String | _Writer) dst, ?Integer copy_length, ?Integer src_offset) -> Integer
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[note] Make the argument types more accurate according to the API doc:

IO.copy_stream copies src to dst. src and dst is either a filename or an IO-like object. IO-like object for src should have #readpartial or #read method. IO-like object for dst should have #write method.

-def self.copy_stream: (_Reader src, _Writer dst, ?Integer copy_length, ?Integer src_offset) -> Integer
+def self.copy_stream: ((String | _Reader | _ReaderPartial) src, (String | _Writer) dst, ?Integer copy_length, ?Integer src_offset) -> Integer

I've added a new interface _ReaderPartial (like _Reader) to core/builtin.rbs.
Please teach me if there is a better name than _ReaderPartial.

@ybiquitous ybiquitous marked this pull request as ready for review December 16, 2020 16:58
Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

💮

@soutaro soutaro merged commit ea7ff9f into ruby:master Dec 17, 2020
@ybiquitous ybiquitous deleted the improve-io-methods branch December 17, 2020 00:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants