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

Add guideline about omitting super arguments #941

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

Conversation

Earlopain
Copy link

For rubocop/rubocop#12427 which is the RuboCop implementation of this. I'll just copy the examples over:

def some_method(*args, **kwargs)
  super(*args, **kwargs)
end

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

# 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)
end

# good - forwarding no arguments
def method(*args, **kwargs)
  super()
end

There's an interesting interaction with blocks that I wasn't previously aware of: they are just always passed, unless explicitly opted out through &nil. Demonstration:

class A
  def foo(*args, **kwargs)
    if block_given?
      pp yield
    else
      pp "No Block"
    end
  end
  
  def bar(...)
    if block_given?
      pp yield
    else
      pp "No Block"
    end
  end
end

class B < A
  def foo(*args, **kwargs)
    super(*args, **kwargs)
    super()
  end
  
  def bar(*args, **kwargs, &blk)
    super(*args, **kwargs, &blk)
    super()
  end
end

B.new.foo("foo", c: "c", d: "d") { "FOO" }
B.new.bar("bar", c: "c", d: "d") { "BAR" }
# => Never prints "No Block"

I've added a callout for this at the end.

Copy link
Member

@pirj pirj left a comment

Choose a reason for hiding this comment

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

Thanks! This looks like a nice addition to the guide. I have some questions, though.

The question to when declare parameter names in the method signature or to use the … syntax or an anon splat is not directly related, and would better be a separate guideline if we happen to end up shaping it.

@@ -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.

# 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

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?

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


# 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

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.

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

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?

@pirj
Copy link
Member

pirj commented May 22, 2024

@zverok, what is your take on this?

@Earlopain
Copy link
Author

A user made this issue report: rubocop/rubocop#12926. A bit unexpected I think. The following snippet demonstrated the problem:

class A
  def positional_arg(a)
    puts a
  end

  def block_arg(&block)
    yield
  end
end

class B < A
  def positional_arg(a = nil)
    a = 'b'
    super
  end

  def block_arg(&block)
    block = proc { puts 'b' }
    super
  end
end

B.new.positional_arg('a')
B.new.positional_arg
B.new.block_arg { puts 'a' }
B.new.block_arg

The output I would expect is b four times but it actually is b b a and LocalJumpError. Would anyone know if that is expected behaviour? I would almost say it is a bug but then again block arg handling is plenty different from other args already.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants