- 
                Notifications
    
You must be signed in to change notification settings  - Fork 5.5k
 
[DOC] Fix grammar errors, typos, and improve readability of string.rb #12151
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] Fix grammar errors, typos, and improve readability of string.rb #12151
Conversation
| # | ||
| # - A first argument, +pattern+ (string or regexp), | ||
| # that specifies the substring(s) to be replaced. | ||
| # - A first argument, +pattern+ (String or Regexp), | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Capitalizing changes like this will make them link to the relevant classes, which is an improvement IMO.
| # - Inherits from {class Object}[rdoc-ref:Object@What-27s+Here]. | ||
| # - Includes {module Comparable}[rdoc-ref:Comparable@What-27s+Here]. | ||
| # - Inherits the {Object class}[rdoc-ref:Object@What-27s+Here]. | ||
| # - Includes the {Comparable module}[rdoc-ref:Comparable@What-27s+Here]. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, if in the future we can enable the --embed-mixins flag added by @flavorjones in ruby/ruby, these methods will be added to the String class's doc directly.
| # - #chomp: Returns a copy of +self+ with a trailing record separator removed, if found. | ||
| # - #chop: Returns a copy of +self+ with trailing newline characters or the last character removed. | ||
| # - #squeeze: Returns a copy of +self+ with contiguous duplicate characters removed. | ||
| # - #[] (aliased as #slice): Returns a substring determined by a given index, start/length, range, regexp, or string. | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it takes regexp too so I just added it:
irb(main):001> s = "foobar"
=> "foobar"
irb(main):002> s[/foo/]
=> "foo"
| 
           @jeremyevans @peterzhu2118 would you mind giving this a look? Thx  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. I do have some suggestions for minor improvements, but I don't need to review again before merging.
| 
           @jeremyevans Thanks for the suggestions!  | 
    
7de8da9    to
    38f487d      
    Compare
  
    Because this file's comments have one extra space after the `#` sign, almost every line is updated. IMO, it's better to address this issue in one go.
38f487d    to
    81be59e      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. Only one change I would make.
Co-authored-by: Jeremy Evans <code@jeremyevans.net>
Because this file's comments have one extra space after the
#sign, almost every line is updated. IMO, it's better to address this issue in one go.To ignore the whitespace changes, please use this link for review.