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] Adjust some new features wording/examples. #9183
Conversation
variable.c
Outdated
* | ||
* c.new # => #<MyClass(with description):0x0....> | ||
* c.instance_method(:foo) # => #<UnboundMethod: MyClass(with description)#foo() ...> | ||
* c.new.method(:foo) #<Method: MyClass(with description)#foo() ...> |
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.
Missing # =>
in this line
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.
Do we need an example regarding the method? It seems trivial. I don't think the code examples in the documentation should be more complicated than necessary.
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.
@mame My point was to demonstrate that all related objects are affected (as an emphasis on "why would this be useful").
Though we probably can just leave one of the two lines (instance_method
vs method
), did you mean that?
FYI: |
3457741
to
a3473f4
Compare
@jeremyevans I tried to address all of your concerns. I also added 3 more small commits (listed under |
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 almost ready. Just a couple changes requested.
eval.c
Outdated
* | ||
* module M | ||
* refine String do | ||
* def indent(level) |
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.
Best remove the method definition here, it doesn't affect the result and is unrelated to the method being documented.
variable.c
Outdated
@@ -189,15 +189,25 @@ is_constant_path(VALUE name) | |||
* m.set_temporary_name(nil) # => #<Module:0x0000000102c68f38> | |||
* m.name #=> nil | |||
* | |||
* n = Module.new | |||
* n.set_temporary_name("fake_name") | |||
* c = Class.new do |
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'm with @mame here, the method already has significant examples, I don't think we need to add to them.
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.
OK. My point here was to demonstrate one of the cases where redefining Module#name
and/or Module#inspect
wouldn't fix the related object's #inspect
to be meaningful (and adds to a justification "why somebody needs a new method to set a pretend-name"). But probably, it doesn't look important enough anyway.
a3473f4
to
3af3e18
Compare
@jeremyevans I think I fixed all the comments. So I rebased it from Thanks for the reviews! |
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.
Please squash when merging.
@jeremyevans Do you mean we need to have just one commit with all the changes (because I tried to structure it in a "commit-per-class" fashion)? |
Yes. |
(Part of working on this year's changelog.)
Some adjustments of the wording of the new features docs.
The changes are relatively small, so I packed them into one PR. If this is unacceptable, I can split in several (every change is its own commit), but I hope that it can be reviewed easily.
The changes in this PR:
:rescue
event.>>
usage from class header example (as it is deprecated, we shoudn't encourage it with the initial example).Added Dec 12:
#refined_class
#fchdir
at all)