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
Add documentations to concern methods [ci skip] #35886
Conversation
266a79b
to
a6e01ff
Compare
I modified documentations so that (I believe) they're more useful for end users. |
a6e01ff
to
dce3143
Compare
@kamipo Thank you for your comments. I force-pushed a commit following your advice. |
dce3143
to
7b29bcd
Compare
@@ -137,6 +140,23 @@ def included(base = nil, &block) | |||
end | |||
end | |||
|
|||
# Define class methods from given block. | |||
# You can define multiple +class_methods+ blocks. |
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.
hmm... actually multiple class_methods
works, but seems it is not by design.
Is it worth to mention about multiple class_methods
?
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.
Actually, I found it interesting that we can have multipleclass_methods
while we cannot have multiple included
.
Should it raise an exception when we have multiple class_methods
, for the sake of symmetry?
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.
It seems worth discussing (I'd say less worth to disallow things aren't broken though).
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.
It might be good to put deprecation warning first when we have multiple class_methods
.
If so, it's not a good idea to document multiple class_methods
. What do you think?
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.
We don't always describe all behaviors in the API doc (e.g. #28490 (comment)).
Regardless of whether it is deprecated or not I don't think the multiple class_methods
is valid use case.
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.
Alright, I'll remove multiple case from an example.
7b29bcd
to
f0e5285
Compare
OK, I removed multiple |
# | ||
# private | ||
# | ||
# def bar; puts 'bar'; end |
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.
Can you follow the Coding Conventions?
Indent and no blank line after private/protected.
https://edgeguides.rubyonrails.org/contributing_to_ruby_on_rails.html#write-your-code
# end | ||
# end | ||
# | ||
# Buzz = Class.new.include(Example) |
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.
Buzz = Class.new.include(Example)
is not usual to me.
Can you use actual class Buzz
?
f0e5285
to
8dcce70
Compare
Summary
Add some documentations to concern methods (
included
andclass_methods
).append_features
is:nodoc:
because users don't care about it.