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

Style/ArgumentsForwarding seems a bit heavy-handed #612

Closed
searls opened this issue Mar 5, 2024 · 5 comments · Fixed by #660
Closed

Style/ArgumentsForwarding seems a bit heavy-handed #612

searls opened this issue Mar 5, 2024 · 5 comments · Fixed by #660
Labels
rule change 👩‍⚖️ Suggest a style guide rule change

Comments

@searls
Copy link
Contributor

searls commented Mar 5, 2024

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?

@marknuzz
Copy link

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) 

@searls
Copy link
Contributor Author

searls commented Oct 24, 2024

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?

@bjeanes
Copy link
Contributor

bjeanes commented Nov 10, 2024

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!)

@searls
Copy link
Contributor Author

searls commented Nov 11, 2024

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

@bjeanes
Copy link
Contributor

bjeanes commented Nov 12, 2024

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.

bjeanes added a commit to bjeanes/standard that referenced this issue Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rule change 👩‍⚖️ Suggest a style guide rule change
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants