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

[DOC] Enhanced RDoc for io.c #5451

Merged
merged 6 commits into from Jan 18, 2022
Merged

[DOC] Enhanced RDoc for io.c #5451

merged 6 commits into from Jan 18, 2022

Conversation

BurdetteLamar
Copy link
Member

@BurdetteLamar BurdetteLamar commented Jan 15, 2022

Treats:

  • IO#reopen
  • IO#printf
  • Kernel#printf
  • IO#print
  • Kernel#print
  • IO#putc
  • IO.new
  • IO#set_encoding_by_bom
  • IO.for_fd

@BurdetteLamar BurdetteLamar added the Documentation Improvements to documentation. label Jan 15, 2022
Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Looks mostly good. Please see requested changes.

io.c Outdated
@@ -8159,13 +8171,12 @@ rb_io_printf(int argc, const VALUE *argv, VALUE out)

/*
* call-seq:
* printf(io, string [, obj ... ]) -> nil
* printf(string [, obj ... ]) -> nil
* printf(string, *objects) -> nil
Copy link
Contributor

Choose a reason for hiding this comment

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

The case where the first argument is a string is treated differently than the case where the first argument is not a string. Additionally, the method supports being passed no arguments (doing nothing in that case), So there should be three lines in call-seq, and you should probably have two examples, one for io provided, and one for io not provided (no example needed for no arguments).

Copy link
Member Author

Choose a reason for hiding this comment

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

Have not succeeded in forming an example for printf(io, string [, obj ... ]) . Need help.

Copy link
Contributor

Choose a reason for hiding this comment

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

printf($stderr, "%d", 1)

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated. Can I get by with a 2-line call-seq?

io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
@jeremyevans
Copy link
Contributor

@zverok Since you are interested in improving Ruby's documentation, maybe you can take over reviewing @BurdetteLamar's documentation changes going forward? However, if not, I'm fine continuing to do so.

Copy link
Contributor

@jeremyevans jeremyevans left a comment

Choose a reason for hiding this comment

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

Almost ready. Just one minor change needed.

io.c Outdated
@@ -8159,13 +8171,33 @@ rb_io_printf(int argc, const VALUE *argv, VALUE out)

/*
* call-seq:
* printf(io, string [, obj ... ]) -> nil
* printf(string [, obj ... ]) -> nil
* printf(io = $stdout, string, *objects) -> nil
Copy link
Contributor

Choose a reason for hiding this comment

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

You need a separate call-seq line for the io case. the call-seq you are using is not valid Ruby syntax (cannot have a required parameter between an optional parameter and a splat).

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed.

@BurdetteLamar
Copy link
Member Author

@zverok Since you are interested in improving Ruby's documentation, maybe you can take over reviewing @BurdetteLamar's documentation changes going forward? However, if not, I'm fine continuing to do so.

Don't see @zverok on the reviewers dropdown.

@nobu nobu changed the title Enhanced RDoc for io.c [DOC] Enhanced RDoc for io.c Jan 17, 2022
@zverok
Copy link
Contributor

zverok commented Jan 17, 2022

I'll definitely take a look later today 👍

Don't see @zverok on the reviewers dropdown.

My credentials receiving is in progress yet 😀

@zverok zverok self-requested a review January 17, 2022 08:34
@BurdetteLamar
Copy link
Member Author

@zverok, thanks! Looking forward to our working together.

@BurdetteLamar
Copy link
Member Author

@jeremyevans, is this ready to merge?

@zverok
Copy link
Contributor

zverok commented Jan 17, 2022

@BurdetteLamar Can you wait till I have a look (today at the evening)?
I think I have some cosmetic notes, but it is still workday in my timezone, I'll be able to look in 1.5h or so

@BurdetteLamar
Copy link
Member Author

@BurdetteLamar Can you wait till I have a look (today at the evening)? I think I have some cosmetic notes, but it is still workday in my timezone, I'll be able to look in 1.5h or so

Sure thing. Just wondering, what timezone are you in?

@zverok
Copy link
Contributor

zverok commented Jan 17, 2022

GMT+2. I typically have my OSS time after 8pm my timezone (e.g. 6pm UTC)

Copy link
Contributor

@zverok zverok left a comment

Choose a reason for hiding this comment

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

Thanks for your work!

I left a few comments and tried to explain why some areas require more attention from us.

io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Outdated Show resolved Hide resolved
io.c Show resolved Hide resolved
io.c Outdated
* ios.set_encoding_by_bom -> encoding or nil
* set_encoding_by_bom -> encoding or nil
*
* If the stream begins with a BOM, consumes the BOM
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shoud explain what's BOM here, it is not a common knowledge and not exactly easy to Google :)

Copy link
Member Author

Choose a reason for hiding this comment

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

Added link to wikipedia BOM.

io.c Show resolved Hide resolved
@BurdetteLamar
Copy link
Member Author

@zverok, if ok with your, I'd like to do these mods and re-reviews piecemeal, while If get the feel of some new strategies.

Copy link
Contributor

@zverok zverok left a comment

Choose a reason for hiding this comment

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

Thanks a lot for all your hard work and patience ❤️
I suggested a couple of the typo fixes; other than that, all looks awesome.

io.c Outdated
* Writes the given object(s) to <em>ios</em>. Returns +nil+.
* Writes the given objects to the stream; returns +nil+.
* Appends the output record separator <tt>$OUTPUT_RECORD_SEPARATOR</tt>
* <tt>$\\</tt>), if it is not +nil+.
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
* <tt>$\\</tt>), if it is not +nil+.
* (<tt>$\\</tt>), if it is not +nil+.

io.c Outdated
* - Converts via its method +to_s+ if not a string.
* - Writes to the stream.
* - If not the last object, writes the output field separator
* <tt>$OUTPUT_FIELD_SEPARATOR</tt> (<tt>$,</tt> if it is not +nil+.
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
* <tt>$OUTPUT_FIELD_SEPARATOR</tt> (<tt>$,</tt> if it is not +nil+.
* <tt>$OUTPUT_FIELD_SEPARATOR</tt> (<tt>$,</tt>) if it is not +nil+.

io.c Outdated
* set_encoding_by_bom -> encoding or nil
*
* If the stream begins with a BOM
* ({byte order marker}[https://en.wikipedia.org/wiki/Byte_order_mark],
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
* ({byte order marker}[https://en.wikipedia.org/wiki/Byte_order_mark],
* ({byte order marker}[https://en.wikipedia.org/wiki/Byte_order_mark]),

@BurdetteLamar BurdetteLamar merged commit ab85c5e into ruby:master Jan 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Documentation Improvements to documentation.
3 participants