Start enforcing parens on macros with arguments

[gmalette]

This is a follow up on #99, more specifically to this comment.

This PR removes the IgnoreMacros: true for the Style/MethodCallWithArgsParentheses cop, effectively meaning that we'll start asking devs to use parentheses around method calls.

Before this PR goes out, we need to:

• [ ] ensure communication that this change is coming down
• [ ] merge https://github.com/Shopify/shopify/pull/183789 on Core
• [ ] encourage devs to apply the same diff to their own projects beforehand

[exterm]

Leaving my opinion here to balance things out... I am in favour of using parentheses everywhere, as I think it can improve consistency considerably. Right now, method calls without parentheses is something that "almost always" works in Ruby. Ruby code bases are littered with exceptions where parentheses need to be used.

I don't even want to mention the cases where parentheses should be used but are not, because the rules of when to use them are too hard to understand. I think Guillaume does a great job of presenting that reasoning in his blog post.

I think it is a given that we'll have strong opinions on either side of this argument. The most interesting question is how we can "disagree and commit" on anything here.

[gmalette]

@dcopeland you provided your opinion but you didn't explain why you hold this opinion, making it very hard to argument. Can you clarify these few points please?

I think it makes our code less consistent [...] internally

Right now we have multiple ways of writing them, I don't see how having one way makes it less consistent, can you explain what you mean?

solves a problem that I agree with in theory but don't think is a real problem in practice

Can you elaborate on this?

uses the wrong tool to solve it

Can you tell us why it's the wrong tool?

[gmalette]

ping @dcopeland in case you didn't see my reply above ^

[dcopeland]

@dcopeland you provided your opinion but you didn't explain why you hold this opinion, making it very hard to argument. Can you clarify these few points please?

I think it makes our code less consistent [...] internally

What I mean specifically by internally is that I think there will continue to be a lot of inconsistency in style across Shopify ruby repositories, making it harder for developers to switch between them.

solves a problem that I agree with in theory but don't think is a real problem in practice Can you elaborate on this?

I mean that I don't believe it's very common for a developer to actually encounter these problems and spend time figuring them out, or for actual bugs or outages to occur in production because of them.

uses the wrong tool to solve it Can you tell us why it's the wrong tool?

It happens too late in the tool chain. You're trying to help developers avoid writing code that doesn't work as they expect. But by the time the style checks occur the code is complete, working and tested.

I think what you really want is a change to the language syntax, and that's clearly not a possibility.

[exterm]

by the time the style checks occur the code is complete, working and tested.

That depends on how you set up your tool chain. Personally, I find the workflow you describe above intensely annoying. That's why I let my editor check the style rules while I type.

I am not sure how many people do that, but it seems to be the best way to handle any kind of style rules, from my perspective.

[gmalette]

I mean that I don't believe it's very common for a developer to actually encounter these problems and spend time figuring them out, or for actual bugs or outages to occur in production because of them

My blog post describes that this initiative started because of bugs in our code base, 27 to be precise, but I don't think I ever linked to a PR, so here it is. Unfortunately there's no way I know of to know if or exactly how many more occurrences there would be, nor how many were actually prevented by people banging their heads trying to make tests pass. What we do know for a fact that they do happen.

It happens too late in the tool chain.

Well, maybe the very first time they write code against this rule, iff they don't use editor linters. As soon as they're made aware of this style change, it will be the very first thing that happens in the lifecycle: writing code.

What I mean specifically by internally is that I think there will continue to be a lot of inconsistency in style across Shopify ruby repositories, making it harder for developers to switch between them.

I agree. One of the retro action item I have not carried through yet is to ask the Technical Leadership Team if the styleguide is required or optional. Currently, repos are not even required to follow the style guide. If you feel strongly about this, I suggest you voice your opinion with the TLT such that the styleguide becomes applied to every repo.

[dcopeland]

@exterm I think we should think about changes that impact all developers from the perspective of the least sophisticated usage.

[dcopeland]

@gmalette To be clear, I fully agree with you about the terrible behaviour of assert + block. I consider that unrelated. You fixed that by changing the behaviour of assert when a block is passed, not by changing the style guide. I've also read your blog post on this more than once and I think you're entirely correct about those examples.

I still don't believe the lack of parentheses is costing us a lot of developer time or production availability.

[DerekStride]

I wanted to weigh in to say that I'm in favour of this change.

I still don't believe the lack of parentheses is costing us a lot of developer time or production availability.

It may not be costing us a lot, however, there has been clear evidence that it has caused some issues. To me that's enough reason to add the style change, there aren't many side effects, I don't think we should care about cross-repo style-consistency as much as removing a potential source of incorrect code.

[dcopeland]

@DerekStride Do you have examples of this?

[DerekStride]

@dcopeland I was referring to the PR @gmalette referenced above https://github.com/Shopify/shopify/pull/179085.

[gmalette]

You fixed that by changing the behaviour of assert when a block is passed, not by changing the style guide.

Given all methods implicitly accept blocks and would present the same behaviour, are you suggesting you should instead change all methods to accept then reject blocks, like I did in https://github.com/Shopify/shopify/pull/179085 ?

[gmalette]

To be clear, I consider the changes to the assert method in https://github.com/Shopify/shopify/pull/179085 to be stopgap measures. They're not a fix in any way and I think it's nonsense to extend that measure to the entire codebase; it would introduce immeasurable amount of unnecessary complexity.

[dcopeland]

I confidently maintain all of my objections above but I don't think further discussion will be helpful. Ultimately I think we agree on the facts but differ on the importance we place on them, which means this difference is not reconcilable.

Since I'm the only one taking this position I think you have no choice but to proceed over my objections, and I have no choice but to disagree and commit.

[kmcphillips]

Since I'm the only one taking this position

You're not. There are only 4 participants on this PR.

Ultimately I think we agree on the facts but differ on the importance we place on them

I share all these views. I understand the reasoning, but disagree that the tradeoffs of (subjectively) less readable code and inconsistencies with widely used community conventions are worth it.

[dcopeland]

FWIW I support changing the behaviour of assert when passed a block because the expectation that assert would take a block is totally reasonable. I don't think the same problem exists elsewhere. Ruby developers in my experience assume a method will not properly handle a block unless it's specifically designed to use one or they've verified it does. This uncertainty is widely accepted.

[swalkinshaw]

I'm also against this change. All the reasons in favour of this valid and concrete which makes it harder to argue the other way. But as @kmcphillips said, things like consistency with the larger community are important. A lot of developers talk about Ruby code being beautiful, or bringing them joy to write, and I think the code style this rule is changing is part of that.

[volmer]

@kmcphillips @swalkinshaw @dcopeland Could you please clarify if you're against enforcing parens on macros (the change proposed in this PR) or for all method calls (which would require a different PR perhaps)?

[swalkinshaw]

I was more okay with the previous change (that was "for all method calls" right?). I'm more specifically against this PR.

[gmalette]

Ruby developers in my experience assume a method will not properly handle a block unless it's specifically designed to use one or they've verified it does

I don't think I understand what you're saying, or you misunderstand the problem with all methods in Ruby, not just assert. Either way I can rephrase the problem.

All methods accept a block, period. That's not the programmer's choice to make. How that block is handled, is (as, I think, you pointed out). Thus the problem becomes: how does someone get feedback when they do the wrong thing? It's entirely possible they did not mean to pass a block to the method, it just happened that way because they didn't use parentheses.

Take this method called double taking an array, and let's pretend it's unreasonable for us to believe that it takes a block.

def double(array)
  array.map { |x| x * 2 }
end

Now take a programmer who is not aware of block precedence (most people aren't). They could very well write this (some people did):

double [1, 2, 3, 4].select do |x|
  x.even?
end

They didn't expect double to take a block, in fact they didn't think the block would be sent to double.

(that was "for all method calls" right?)

No, eh... the previous PR was... I don't know how to qualify it. "enforce parens sometimes" maybe ?

@swalkinshaw @kmcphillips can you clarify where you're OK with enforcing parens and where you aren't? ELI5 pls

[swalkinshaw]

Basically I always use parens unless its class-macros or DSLs. I was okay-ish with the other change we made, but I'm less okay with this one.

Here's a snippet of code to explain my personal prefs:

# frozen_string_literal: true
module GraphApi
  module Admin
    module HasMetafields
      include GraphApi::InterfaceType

      required_access false # no, since its a macro
      description 'Represents information ...' # ditto

      field :metafield, Metafield, null: true do |field| # macro/DSL as well, no parens
        field.description = 'The metafield associated with the resource.'
        field.argument(:namespace, # I also wouldn't use parens here, to be consistent with the "parent", but this is enforced now
          :string,
          required: true,
          description: 'Container for a set of metafields (maximum of 20 characters).')
      end

      def metafield(namespace:, key:) # yes, always for a method definition
        return unless object.respond_to?(:metafields) # yes, just a normal method call
        object.metafields.find_by(namespace: namespace, key: key) # ditto
      end
    end
  end
end

From my experience writing Ruby, my subjective rules are pretty much the community standard. However, I admit it can be confusing and inconsistent.

[gmalette]

Since there are no macros (it's just method calls) nor DSL (it's just Ruby) in Ruby , can you clarify what you mean by those please?

[swalkinshaw]

The title of this PR (which you created) uses the term macros and the Rubocop rule uses the term too. So I'm not sure if you're trying to get me to supply my own definition of those terms, but I don't have one that differs in this context.

[wvanbergen]

A little rant on consistency. I see it being referred to as a goal, but it is a means to an end. Why do we want consistency? The main reason I found after some googling is higher readability, which makes It simpler and/or easier for people to understand a codebase and start working on it, fix bugs, re-use the code, etc..

Note that easy is a relative concept: it depends on your starting point. Simpler on the other hand is absolute: it doesn’t depend on your current level of knowledge or starting point.

I think this case is a good example of simple and easy being in conflict with each other. I don’t think that include Foo is inherently more readable than include(Foo): it’s only more readable if you are currently used to include Foo, i.e. easier rather than simpler. So this style guide change makes it slightly harder (opposite of easy) to read (and write) code if you are used to current conventions.

At the same time, it makes it simpler to read (and write) code, because it reduces ambiguity and complexity. You don’t have to know about the subtleties of block argument binding and the differences between do … end vs. { … } to read code when parentheses are present.

I am all in camp “simple” over camp “easy”. Having less ambiguity is a lasting benefit which will prevent bugs, while the pain of having to get used to this change is temporary. Moreover, the pain only applies to people that are used to the current conventions; we have many people joining Shopify that have never written Ruby before who will not suffer from this pain at all.

[wvanbergen]

FWIW I support changing the behaviour of assert when passed a block because the expectation that assert would take a block is totally reasonable.

The problem here is not that developers think that assert accepts a block yes or no. The problem is the ambiguity in the Ruby syntax that caused assert to receive a block argument that the developer intended to pass to another method.

[dcopeland]

@volmer I'm in favor of preventing this PR from merging and reverting #99 .

@gmalette I understand. I'm saying that I believe there are lots of interfaces in Ruby that have undefined or unexpected behaviour when passed blocks (certainly in our own code) and in my experience Ruby developers are not surprised by this.

[rafaelfranca]

I think it would be harder to explain better than @wvanbergen why I think this change is important and beneficial to our codebase right now and in the long term. Requiring parentheses will make writing and reading code in Ruby simpler. The benefits of this change in my opinion overweight any downside presented until now.

Like Scott's comment above explain well, in favor we have valid in concrete reasons on why we should require parentheses. Opposing to that we have subjective and felling-based reasoning. While I'm understand those feelings, I can't say we should make a decision based on that alone given we do have concrete reason in the other side.

That being said, while I prefer to write and read code without parentheses and I'm comfortable with Ruby's precedence of operators and block, I still think we should enforce parentheses in all methods calls, including macros.

[wvanbergen]

Also we need to care for the stability of Shopify's Ruby Style Guide, and this change in particular disrupts it considerably.

@volmer At what point to you think preventing bugs takes precedence over stability of the style guide and familiarity with the rest of the Ruby ecosystem?

[rafaelfranca]

BTW, related to this discussion https://bugs.ruby-lang.org/issues/15554

[volmer]

At what point to you think preventing bugs takes precedence over stability of the style guide and familiarity with the rest of the Ruby ecosystem?

It would really depend on the case, of course. For the discussion on hand (parentheses on macros) the bug prevention doesn't justify the friction and the compromise in terms of style.