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

Prefer reduce over inject #237

Merged
merged 1 commit into from
Nov 20, 2014
Merged

Prefer reduce over inject #237

merged 1 commit into from
Nov 20, 2014

Conversation

derekprior
Copy link
Contributor

reduce is gaining popularity in ruby and provides symmetry with our
recommendation to use map.

@croaky
Copy link
Contributor

croaky commented Oct 18, 2014

👍

@mcmire
Copy link

mcmire commented Oct 19, 2014

I don't have an opinion one way or the other. I guess this makes sense, though, just for the symmetry (and crossover from JavaScript). I've always thought inject was a weird name for a method. I wouldn't be opposed to changing.

@BlakeWilliams
Copy link
Contributor

👍

@sikachu
Copy link
Contributor

sikachu commented Oct 20, 2014

By "gain popularity", can I see the prove? Maybe only haskeller who also writing Ruby?

[master][~/Projects/rails] ag "reduce\(" | wc -l
       3
[master][~/Projects/rails] ag "inject\(" | wc -l
      36
[master][~/Projects/rails] 

I use inject in all of the code I've written, and it's in my muscle memory. I'm sure a lot of people do to.

I don't understand why this change has to be in place. It feels bikeshedding to me. If people prefer to use reduce, then go for it in that particular project.

I'm voting for removing this rule, and use whatever you want that floats your boat.

@sikachu
Copy link
Contributor

sikachu commented Oct 20, 2014

Also, quote @pbrisbin from the previous PR:

I'm 👎 because I see no objective reason one way or the other. For me, this all comes down to arbitrary convention followed for consistency. I see no need to change things from one arbitrary choice to another.

I think that still applies to this change. I don't see a need for this change either.

@gylaz
Copy link
Contributor

gylaz commented Oct 20, 2014

Perhaps we should remove our preference altogether?

@jferris
Copy link
Contributor

jferris commented Oct 20, 2014

We had a very similar discussion in #169. The end result was that, in general, people preferred reduce, but the change wasn't enough of an improvement that it was worth invalidating our existing code. Do people now feel that using reduce is so much better that it's worth having a rule that's inconsistent with our existing code?

Perhaps we should remove our preference altogether?

Although I don't think it matters much which name we use, I prefer to have consistency. Is there any benefit to using different names in different places?

@pbrisbin
Copy link
Contributor

As usual, it seems I agree with past @mike-burns

... since we already have a rule, and since the whole point of our guides is that we can argue things once and never have to think about it again, and since we've already argued this: not sure why we should merge this.

@derekprior
Copy link
Contributor Author

I don't understand why this change has to be in place. It feels bikeshedding to me. If people prefer to use reduce, then go for it in that particular project.

We can't. Because there's this guideline. It is a form of bikeshedding. But this is the repository for bikeshedding.

but the change wasn't enough of an improvement that it was worth invalidating our existing code.

I can't think of a single stylistic change that would be worth this. If that's the bar we might as well freeze all modifications to style, right? Is that really the bar?

the whole point of our guides is that we can argue things once and never have to think about it again

I don't think that is... or should be, anyway... the point of our guides. Strong opinions, loosely held and all. We should always be open to re-examining past decisions.

If starting a new project today, I would prefer to use reduce. As mentioned in the commit, it provides symmetry with map but it's also more intention revealing. When I write code with inject and people are unfamiliar with the method, the name is more of a stumbling block than it would be if we were using reduce. This is why I opened the PR in the first place. reduce this array to something makes sense. I've never heard a reason for inject as the name for this other than it rhymes with all the other methods.

I think this is a pretty minor change. If people prefer inject on its merits, then I think that's fine. Let's just move on. But I think we should be examing guides changes with that lens.

@jferris
Copy link
Contributor

jferris commented Oct 20, 2014

I can't think of a single stylistic change that would be worth this. If that's the bar we might as well freeze all modifications to style, right? Is that really the bar?

In some cases, we're creating a new guideline where there wasn't one before. The bar is easier here, because it's not creating inconsistency, as the old code was already inconsistent.

When reversing an earlier decision, I think the bar has to be higher. It's still fuzzy, but I think we have to decide as a group that we dislike the current guideline more than we'd dislike having (hopefully temporary) inconsistency during the transition.

A recent example was the transition to RSpec's new default syntax, using allow and expect. Although the transition was (and still is) annoying, it's clearly where RSpec is going in the future, and it makes sense to keep up.

In this case, I don't personally have strong feelings about reduce or inject, and I haven't seen confusion or difficulties with inject which I think would be resolved by using reduce. If other people find inject really ugly or confusing, or we feel like our conventions are out of touch with the rest of the Ruby community, I still think it's worth considering a change.

@gylaz
Copy link
Contributor

gylaz commented Oct 20, 2014

We've put things to votes before. Could that be incorporated into the process of deciding on things like that?

@croaky
Copy link
Contributor

croaky commented Oct 20, 2014

reduce is more in line with community conventions:

https://github.com/bbatsov/ruby-style-guide#map-fine-select-reduce-size

In this case, I doubt the transition from inject to reduce on old
codebases will be an issue, similar to allow or double quotes.

I agree with Derek about "strong opinions, loosely held" but also don't
think we need to make this thread about the grander goals of the guides
repo. It seems most important to making this decision quickly one way or
the other quickly, which feels like the goal Mike and Pat are really asking
for in their comments. We obviously don't want to flip flop on every rule
all the time but again, let's not make this discussion about grander
concepts than this one change, which feels like we can decide quickly on
its own merits.

On Mon, Oct 20, 2014 at 8:58 AM, Greg Lazarev [email protected]
wrote:

We've put things to votes before. Could that be incorporated into the
process of deciding on things like that?


Reply to this email directly or view it on GitHub
#237 (comment).

@jferris
Copy link
Contributor

jferris commented Oct 20, 2014

I created a poll so that we can see where things currently stand: https://docs.google.com/a/thoughtbot.com/forms/d/1XFIOLZju440wUiVLPd2jIz2iRlFYg2vCm9-l9lFLZc8/viewform?usp=send_form

@jferris
Copy link
Contributor

jferris commented Nov 18, 2014

Here are the current results, after about a month of voting:

reduce-vs-inject

It doesn't seem like there's a strong enough sentiment that it would be worth switching to reduce. If we don't get a stronger consensus this week, I'll close.

@jferris
Copy link
Contributor

jferris commented Nov 20, 2014

Latest results:

reduce-vs-inject-update

7-2 is a pretty strong consensus, so I guess we'll make the switch.

@derekprior feel free to merge.

`reduce` provides symmetry with our already-preferred `map`.
Additionally, it has a more intention-revealing name and is more
approachable for people unfamiliar with it. "I am reducing this array to
something". `inject` doesn't make the same sense.
@derekprior derekprior merged commit 0b64873 into master Nov 20, 2014
@derekprior derekprior deleted the dp-reduce branch November 20, 2014 18:34
jferris added a commit to houndci/hound that referenced this pull request Nov 20, 2014
This is more inline with community conventions and is the new thoughtbot
preference.

thoughtbot/guides#237
@jferris
Copy link
Contributor

jferris commented Nov 20, 2014

houndci/hound#491

@mcmire
Copy link

mcmire commented Nov 20, 2014

Ch-ch-changes.......

(you're welcome)

@halogenandtoast
Copy link
Contributor

David Bowie is always welcome.

jferris added a commit to houndci/hound that referenced this pull request Dec 4, 2014
This is more inline with community conventions and is the new thoughtbot
preference.

thoughtbot/guides#237
composerinteralia added a commit that referenced this pull request Jun 12, 2020
I know we have had some back on forth on these (e.g. #169 and #237).
These preferences were fine when we could automate fixing them with
RuboCop. But as we move to standard I think we should buy fully into
[standard's opinion on these aliases][standard opinion]. If we don't
agree with that opinion, we should discuss the issue on standard instead
of in this guide.

[standard opinion]: standardrb/standard#39 (comment)
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.

9 participants