On Mon, Apr 11, 2016 at 5:58 PM, Jeff Gilbert <jgilb...@mozilla.com> wrote:
> On Mon, Apr 11, 2016 at 4:00 PM, Bobby Holley <bobbyhol...@gmail.com>
> wrote:
> > On Mon, Apr 11, 2016 at 2:12 PM, Jeff Gilbert <jgilb...@mozilla.com>
> wrote:
> >> I propose that if you're in a part of the codebase where you can't
> >> tell if it's an enum or a function pointer (regardless of the fact
> >> that the compiler can't confuse these), you should do more looking
> >> around before working on it, much less reviewing changes to the code.
> >> If you can't tell if it's an enum, you're not going to be able to
> >> effectively make changes, so buckle up and do some looking around.
> >
> >
> > I strongly disagree with this statement and the sentiment behind it. The
> > ability for certain people to be able to quickly review changes in code
> that
> > they don't have paged into working memory is vital to the survival of
> Gecko.
>
> Well I disagree in the long term. If we are going to survive, we
> *must* minimize such requirements. Skimping on code review should be
> low on our list of options, given its propensity to create bugs and
> accrue technical debt in the name of moving fast in the short term.
>
> It's a tough job. I definitely understand. But good review is a
> necessity. We have a number of problems that are "harsh realities",
> but I hope we're trying to fix what we can, not optimizing assuming
> failure.
>
> > Mozilla is very bad at distributing review loads. There are literally a
> > handful of people holding the ship together (and smaug is one of them).
> It
> > is not a fun job, and the bus-factor is frighteningly low. This means
> that
> > we should absolutely give more weight to the preferences of top reviewers
> > because:
> > (a) We owe it to them, and
> > (b) We want to make it easier for other people to join their ranks.
>
> Why do we have to be bad at distributing review loads? Is it a tooling
> problem? And on-boarding problem? Why can't we reduce our need and/or
> spread the load?
>
It's a hard problem we should do our best to address, but probably not in
this thread.
>
> It's not pleasant to try to discuss these things only to be told that
> I don't review enough code to matter. A bunch of us review code, and
> yes, some more than others. I don't know where to find actual data,
> but there is a panel helping with MozReview feedback whose members
> were selected from 20 people chosen by virtue of being "top reviewers
> by volume", a group which to which smaug, I, and at least 18 others
> belong.
>
To be very clear, I'm not suggesting you don't review a lot of code, or
that your vote doesn't matter. I am not saying anything about you
specifically. Rather, I am challenging your claims that:
(a) We shouldn't optimize for reviewers over developers, and
(b) Somebody reviewing code they don't have paged-in isn't doing a good job.
> There are people (including smaug) who review more code than me.
IMO, the thing that makes people like smaug and bz different from other
reviewers is not just the number of reviews they do, but the _breadth_ of
code they're expected to review. It's impossible to keep all that code
paged in, which is why consistency is so helpful.
> Their
> opinions *do* have more weight than mine. However, I hope I my
> arguments are still heard and granted what weight they are entitled
> to, particularly since many people I talk with in person are strongly
> opposed to changes here and elsewhere, but reluctant to join these
> often-frustrating discussions.
>
> It's always better when we have discussions based on data rather than
> opinions, but when we're down to opinions, it's really hard to judge
> support via email. There's no good "+1" option without spamming, and
> many people just don't want to deal with the stress that merely
> following these discussions can incur.
>
I totally agree. In my perfect world, the workflow would look like this:
(1) Somebody proposes a change to the style guide.
(2) Discussion ensues. If there's a clear consensus, end the algorithm.
(3) There's not a clear consensus. Somebody writes up a fair summary of the
pros and cons raised in the thread, ideally along with automated analysis
of the current state of the tree (i.e. X% of the enums are currently this
way, Y% that way). They post it to the thread, and ask if any rationale is
missing.
(4) Once the summary is deemed fair, we post it in a new thread, along with
a link to an external polling system. People are invited to vote, and may
optionally add a few characters of text to indicate which piece of
rationale was most instrumental in their decision.
(5) We analyze the results. If the results are pretty clear, end the
algorithm.
(6) If the poll results are still unclear, we hand the results to some
arbiter to decide (jst seems like a good choice). This person decides based
on poll results and pre-existing rationale (i.e. "No New Rationale").
Does that seem reasonable? None of it is hard, it just needs somebody to
shepherd it through.
>
> >> To go completely off the rails, I no longer even believe that all of
> >> Gecko should have a single style.
> >
> >
> > Yes, that is off the rails in this context.
> >
> >>
> >> I think the whole attempt is
> >> increasingly a distraction vs the alternative, and I say this as
> >> someone who writes and reviews under at least three differently-styled
> >> code areas. The JS engine will, as ever, remain distinct
> >
> >
> > Jason agreed to converge SM style with Gecko.
>
> The SM engineers I talked to are dismissive of this. I will solicit
> responses.
>
> >> , as will
> >> third-party code, and also all non-actively maintained files. (most of
> >> our codebase)
> >>
> >> Yes, a formatter could magic-wand us to most of the superficial
> >> aspects of a single code style, but we'd then need to backfill
> >> readability into some of the trickier bits. Sometimes we need to step
> >> outside the prevailing style to keep something reasonably readable.
> >>
> >> But we /already have/ a process for keeping things readable: Code
> reviews.
> >> And if that fails: Module ownership.
> >>
> >> I'm not sure why we hold centralization in such high regard when it
> >> comes to code style. And, if we are continuing to centralize this,
> >> we're going to need to have an actual process for making decisions
> >> here.
> >>
> >> I suspect the goal is to have an oracle we can ask how we should
> >> format code in order to be readable. However, we can't even agree on
> >> it, so the right answer often is indeterminate. However, people within
> >> their modules have a better idea of what works in each case.
> >>
> >> I do think we should have some high-level style guidance, but I think
> >> we're increasingly micromanaging style. Let code review take care of
> >> the specifics here. If it's not readable without prefixes, definitely
> >> consider adding prefixes. Otherwise don't require it.
> >>
> >>
> >> FWIW, I do have a preference for k prefix instead of e prefix, (if we
> >> must have a required prefix) since they are in the same 'constants'
> >> category. 'k' I can be persuaded the usefulness of for enums. 'e', not
> >> so much.
> >>
> >>
> >> On Mon, Apr 11, 2016 at 8:00 AM, Kartikaya Gupta <kgu...@mozilla.com>
> >> wrote:
> >> > Based on all the feedback so far I think the best compromise is to use
> >> > enum classes with the "e" prefix on the values. As was mentioned, the
> >> > "e" prefix is useful to distinguish between enum values and function
> >> > pointer passing at call sites, but is a small enough prefix that it
> >> > shouldn't be too much of a burden.
> >> >
> >> > I realize not everybody is going to be happy with this but I don't
> >> > want this discussion to drag on to the point where it's a net loss of
> >> > productivity no matter what we end up doing. I'll update the style
> >> > guide.
> >> >
> >> > Thanks all!
> >> >
> >> > kats
> >> >
> >> >
> >> > On Sun, Apr 10, 2016 at 11:43 AM, smaug <sm...@welho.com> wrote:
> >> >> On 04/10/2016 05:50 PM, Aryeh Gregor wrote:
> >> >>>
> >> >>> On Fri, Apr 8, 2016 at 8:35 PM, smaug <sm...@welho.com> wrote:
> >> >>>>
> >> >>>> enum classes are great, but I'd still use prefix for the values.
> >> >>>> Otherwise the values look like types - nested classes or such.
> >> >>>> (Keeping my reviewer's hat on, so thinking about readability here.)
> >> >>>
> >> >>>
> >> >>> In some cases I think it's extremely clear, like this:
> >> >>>
> >> >>> enum class Change { minus, plus };
> >> >>>
> >> >>> whose sole use is as a function parameter (to avoid a boolean
> >> >>> parameter), like so:
> >> >>>
> >> >>> ChangeIndentation(*curNode->AsElement(), Change::plus);
> >> >>>
> >> >>> In cases where it might be mistaken for a class, it might make more
> >> >>> sense to prefix the enum class name with "E". I don't think this
> >> >>> should be required as a general policy, though, because for some
> enums
> >> >>> it might be clear enough without it.
> >> >>>
> >> >>
> >> >> Indeed in your case it is totally unclear whether one is passing a
> >> >> function
> >> >> pointer or enum value as param.
> >> >> If the value was prefixed with e, it would be clear.
> >> >>
> >> >> Consistency helps with reviewing (and in general code readability) a
> >> >> lot, so
> >> >> the less we have optional coding style rules, or
> >> >> context dependent rules, the better.
> >> >>
> >> >>
> >> >>
> >> >>
> >> >>
> >> >> _______________________________________________
> >> >> dev-platform mailing list
> >> >> dev-platform@lists.mozilla.org
> >> >> https://lists.mozilla.org/listinfo/dev-platform
> >> > _______________________________________________
> >> > dev-platform mailing list
> >> > dev-platform@lists.mozilla.org
> >> > https://lists.mozilla.org/listinfo/dev-platform
> >> _______________________________________________
> >> dev-platform mailing list
> >> dev-platform@lists.mozilla.org
> >> https://lists.mozilla.org/listinfo/dev-platform
> >
> >
>
_______________________________________________
dev-platform mailing list
dev-platform@lists.mozilla.org
https://lists.mozilla.org/listinfo/dev-platform
