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

*: s/rng/rep/ for vars of type Replica #10421

Merged
merged 1 commit into from
Nov 4, 2016

Conversation

jordanlewis
Copy link
Member

@jordanlewis jordanlewis commented Nov 2, 2016

It's confusing to have variables named rng when their type is Replica and not Range or similar. This is a large cosmetic change that fixes a lot of those occurrences.


This change is Reviewable

@tamird
Copy link
Contributor

tamird commented Nov 2, 2016

LGTM without reviewing

@jordanlewis jordanlewis force-pushed the rep-all-the-things branch 2 times, most recently from e735dbd to 575a750 Compare November 2, 2016 22:31
@RaduBerinde
Copy link
Member

LGTM

Nit: I've usually seen repl for Replica vars.. But feel free to ignore, unless you want to try to do it automatically (e.g generate a diff, s/\<rep\>/repl/g in the patch, then reapply).

@jordanlewis
Copy link
Member Author

There unfortunately seems to be a mix of both, but repl wins out by a bit so I've gone ahead and taken your suggestion.

[21:29]% ag " repl[ \.]" | wc -l
84
[21:30]% ag " rep[ \.]" | wc -l
69

@petermattis
Copy link
Collaborator

We also use r for *Replica vars. And that is the receiver name for all of the methods on *Replica. I personally prefer rep for such vars, but that preference is not strong.

@RaduBerinde
Copy link
Member

:lgtm:


Review status: 0 of 16 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from Reviewable

@tbg
Copy link
Member

tbg commented Nov 4, 2016

LGTM but please don't merge before #10327.

It's confusing to have variables named `rng` when their type is
`Replica` and not `Range` or similar. This is a large cosmetic change
that fixes a lot of those occurrences.
@jordanlewis
Copy link
Member Author

@petermattis r seems a bit terse outside of the method receiver context. I hear your preference for rep but it does seem like there are already more instances of repl - considering the inconsistency here I'm just going to go ahead and merge this, since either one is strictly better than the incorrect rng naming.

@jordanlewis jordanlewis merged commit 450d593 into cockroachdb:master Nov 4, 2016
@jordanlewis jordanlewis deleted the rep-all-the-things branch November 4, 2016 15:06
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.

5 participants