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.

Sign up for GitHub

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 guideline about omitting super arguments #941

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add guideline about omitting super arguments #941

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 36 additions & 0 deletions README.adoc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3216,6 +3216,42 @@ def some_method(&)
end
----

=== Super Forwarding

Copy link
Member

Choose a reason for hiding this comment

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

I’m struggling to understand how this corresponds to the Arguments forwarding section above.

Both that other section and this new one have blind forwarding examples only. There, they suggest using the … syntax, and it is understandable. But only for the blind forwarding case.
But what about cases where only some of the arguments need to be forwarded? Like eg this one with def m(foo, …)? Does it even work?

And moving on to the new addition. This all looks reasonable.
My only concerns are below.

Copy link
Author

Choose a reason for hiding this comment

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

I’m struggling to understand how this corresponds to the Arguments forwarding section above.

It fits thematically at least but I see your point about where I placed it. Perhaps it is better suited to come after "Using super with Arguments" ?

Copy link
Member

Choose a reason for hiding this comment

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

My apologies for not expressing my thoughts clearly.
The place is right, I think this new section fits quite well under the arguments forwarding section, and it relates to it.

My concerns were more related to the existing “arguments forwarding” section, as its only “good example” is something I called “blind forwarding”, ie forwarding of all arguments.
This is not exactly always the case, and the existing section is incomplete and may even be confusing due to lack of coverage of those other cases with its only one good example.

Your addition, in the contrary, is detailed, and we happened to discuss a number of edge cases below, so it’s all good.

I’ll keep this comment open for a potential follow-up to improve the “arguments forwarding” section, but not in the scope of this PR.

Use `super` without arguments when the enclosing methods arguments are the same.

When super is called without arguments and parentheses Ruby will automatically forward the arguments taken by the method.

[source,ruby]
----
# bad
def some_method(*args, **kwargs)
super(*args, **kwargs)
end

Copy link
Member

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 with a named argument? Why are all examples splats?

Copy link
Author

Choose a reason for hiding this comment

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

Not sure why I made them all splats to be honest, there's no deeper meaning behind it. I can add a few more examples, though don't really see the reason. There's nothing special about splat/nonsplat and listing all possible variations is unnecessary imo., would just consistently use normal arguments instead. Wdyt?

# good - implicitly passing all arguments
def method(*args, **kwargs)
super
end
Copy link
Member

Choose a reason for hiding this comment

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

If this is blind forwarding, should we use the … syntax to describe parameters in the method signature? I understand that it is intent-revealing to write all of them. Won’t RuboCop with the default config suggest prefixing them with _?

Copy link
Author

Choose a reason for hiding this comment

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

RuboCop is smart enough to not raise an error here: the arguments are not unused


Copy link
Member

Choose a reason for hiding this comment

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

How do we dissect when to describe params in details, and when an unnamed splat (*) and … is fine?

# good - forwarding a subset of the arguments
def method(*args, **kwargs)
super(*args)
end

# good - calling super with different arguments
def method(*args, **kwargs)
super("foo", *args, **kwards)
Copy link
Member

Choose a reason for hiding this comment

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

I haven’t tried, but would super(“foo”, …) syntax work?

Copy link
Author

Choose a reason for hiding this comment

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

This does work in Ruby 3.1 (I think). ... got introduced and one or two versions later this was made valid syntax

end

# good - forwarding no arguments
def method(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

Again, should this be _args and _kwargs?

Copy link
Author

Choose a reason for hiding this comment

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

See above

super()
end
----

IMPORTANT: Blocks are always passed to the parent method. `&nil` can be passed to `super` if the block should remain local.
Copy link
Member

Choose a reason for hiding this comment

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

What if there are other parameters? What is the recommended way to pass all arguments to super, but not the block? The … won’t work, will it?

Copy link
Author

Choose a reason for hiding this comment

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

If you don't want to pass the block you write &nil, so in these examples it would be super(*args, **kwargs, &nil). super(..., &nil) on the other hand is a syntax error so it doesn't look like you are able to suppress the block with that notation.

Should I make a explicit example instead of just mentioning it here?


=== Private Global Methods [[private-global-methods]]

If you really need "global" methods, add them to Kernel and make them private.
Expand Down