standardrb/standard

Style/ArgumentsForwarding seems a bit heavy-handed

searls opened this issue · 5 comments

Just updated standard to 1.34.0 and saw this failure:

  def set_time_zone(&block)
    Time.use_zone(current_user.time_zone, &block)
  end

Which auto-fixes to this:

  def set_time_zone(&)
    Time.use_zone(current_user.time_zone, &)
  end

I was kinda annoyed by this, and maybe even more annoyed when I realized that only [blk, block, proc] trip the rule.

Am I just being an old crank because I have 20 years of muscle memory writing &blk?

It is worth mentioning that Sorbet currently doesn't allow arguments forwarding, including for block arguments. I'm glad to have found this workaround by using a different block argument name. IMO the rule seems to cause more trouble than it is worth.

Malformed `sig`. Type not specified for argument & [5003](https://srb.help/5003) 

Yeah, I have come to personally dislike being forced into this one, and I noticed as well that when I went back to maintain mocktail I had to add a bunch of #ignore directives.

Personally, I think Standard should be compatible with Sorbet codebases and we should disable this one at least until it is.

@camilopayan what do you think?

Yeah, I have come to personally dislike being forced into this one, and I noticed as well that when I went back to maintain mocktail I had to add a bunch of #ignore directives.

Agreed

I just upgraded standard and the version of Rubocop that came with it (which is not the latest FWIW) rewrote some code from this rule and broke some code:

This original code:

    def method_missing(meth, *args, **opts, &blk)
      if delegated_class_method?(meth)
        opts[:account_id] = @account.id if @account
        opts[:env] = @env.merge(opts[:env] || {})
        opts[:session] = @session.merge(opts[:session] || {})
        @auth_class.send(meth, *args, **opts, &blk)
      elsif delegated_instance_method?(meth)
        internal { send(meth, *args, **opts, &blk) }
      else
        super
      end
    end

Was re-written into:

    def method_missing(meth, *, **opts, &)
      if delegated_class_method?(meth)
        opts[:account_id] = @account.id if @account
        opts[:env] = @env.merge(opts[:env] || {})
        opts[:session] = @session.merge(opts[:session] || {})
        @auth_class.send(meth, *, **opts, &)
      elsif delegated_instance_method?(meth)
        internal { send(meth, *args, **opts, &blk) }
      else
        super
      end
    end

This breaks, because there is still a reference to *args that gets unchanged.

I believe this is rubocop/rubocop#12875, which is fixed in newer versions. Leaving this here just as a breadcrumb to others in similar position.

I haven't been able to upgrade rubocop to a fixed version because the latest standard (understandably) has quite a tight version restriction. If I force a newer Rubocop, bundler actually downgrades standardrb to 1.35.0:

Screenshot 2024-11-11 at 10 33 35 am

For now, I'm going to avoid this standard upgrade until this rule is loosened as I agree that it is faaar too heavy-handed and in many cases worsens readability (obv subjective, but that's my take!)

Ok if @bjeanes is on board then I think I'd support a merge if someone sends a PR to disable it.

Is this just a case of flipping

standard/config/base.yml

Lines 986 to 987 in e05a802

Style/ArgumentsForwarding:
Enabled: true
and
Style/ArgumentsForwarding:
Enabled: true
AllowOnlyRestArgument: true
?

Assuming yes, based on other merged PRs that disable rules.