> On Jan 7, 2017, at 11:39 AM, Pierre-Yves David
> <pierre-yves.da...@ens-lyon.org> wrote:
>
>
>
> On 12/30/2016 08:46 PM, Augie Fackler wrote:
>>
>> On Dec 30, 2016 12:30, "Pierre-Yves David" <pierre-yves.da...@ens-lyon.org>
>> wrote:
>>
>>> On 12/30/2016 06:09 PM, Augie Fackler wrote:
>>>
>>>> On Dec 30, 2016, at 12:04 PM, Rémi Chaintron <remi.chaint...@gmail.com>
>>>> wrote:
>>>>
>>>>> Following discussion with marmoute on IRC, I'll drop the rawrevision()
>>>>> method for now.
>>>>
>>>> OOC, what’s the rationale for preferring the boolean parameter instead of
>>>> making the code structure more self-documenting? I legitimately don’t see
>>>> why the parameter version is preferable design-wise.
>>>
>>> What about you check the IRC log so that remi and I can focus on getting
>>> this things out? The log are still warm and fresh.
>>
>> I *did* read the IRC log, which I’ve quoted at the bottom of this message
>> for future reference
>
> There is more information in the IRC log, than you seems to have extracted.
> Please give it an extra reading. To help with that second reading, I've
> quoted the actual full IRC log of that conversation at the end of that email
> (yours only contained the last half of it).
I’ve done so, and responded inline below. Note that the actual objection I see
was quoted in my mail, so if I’m missing something I’d really appreciate you
taking the five minute to at least point out the line in this conversation I’m
missing where you provide an engineering justification for your opinion.
> My time is limited, so I'm not planning to spend around 1h to write a
> detailed email about that minor review feedback. Especially when there is
> already written material about the topic and no sign that you extracted it.
> If your counter proposal is really important to you, fell free to come back
> with signs that you read the IRC log and a more concrete points than a
> generic blog post that doesn't really match Rémi's code.
I’m getting the sense here that you feel like I’ve been disrespectful of your
time, and I’m sorry if you feel that way. I’m trying to work out what the
misunderstanding here is so I can either drop my proposal with an understanding
of why it’s harmful, or move forward with it.
> Full IRC log about patch 2:
>
>> 17:25 < remi17> durin42: sorry if i drop off the channel, currently hacking
>> in a car between NYC and montreal
>> 17:29 < remi17> durin42: I'm having trouble to understand if you prefer the
>> rawrevision() method or explicitly use raw=True?
>> 17:37 < durin42> I prefer rawrevision()
>> 17:37 < durin42> because boolean parameters are typicall bad, and in this
>> case I can even articulate why I don't like it :)
>> 17:41 < remi17> Yeah, as I mentioned on the email thread, we can make this
>> better for revision() by refactoring revision() and rawrevision() to use
>> _revision() (that does the actual work but doesn't do the hash
>> check/flagprocessing) instead and directly invoke
>> processflags() with the right arguments
>> 17:41 < remi17> It'll work well for revision but refactoring _addrevision
>> will be another beast entirely, and I'll have to clean it up from censor
>> before attempting it
>> 17:42 < remi17> Death to all optional booleans still
>> 17:47 < marmoute> remi17: resuming writing things about patch 4 now
>> 17:50 < marmoute> remi17: not sure what's all that hate about the boolean
>> raw is about. your new method is actually just using it.
>> 17:52 < marmoute> remi17: I would prefer we drop the new method. Adding a
>> new parameter is significantly less complex than adding a a new method. And
>> in this case. We have the new parameter in all cases
>> 17:53 < marmoute> So given how rare the 'raw' favor is compared to the
>> normal one, I prefer that we drop the newmethod and stick to the new
>> parameter only.
So, this is one place where you disagree, and it’s not actually addressing my
point (I’ll elaborate later).
>> 17:55 < remi17> marmoute: I honestly don't care much, this is all about code
>> styles and I see your point about the argument being there in the first
>> place. One point I'd still consider is that the right way forward is
>> probably to refactor this part in a subsequent
>> patch to keep only revision() and rawrevision(), getting rid
>> of the argument entirely
>> 17:57 < marmoute> remi17: if rawrevision was a whole complex and different
>> things on its own I would get why we had a new method.
>> 17:57 < remi17> You and durin42 seem to disagree on what's best here, but a)
>> if I get any cycles on this, this will be refactored later on anyway (but
>> I'd understand if you do not want to bet on my cycles) b) it's not something
>> I'm feeling strongy enough about to push
>> either way c) I'd rather coin flip rather than having you
>> guys losing time on this specific bit :)
>> 17:58 < remi17> marmoute: I completely get your argument, it's 100% code
>> style tastes/opinions at that point
>> 17:58 < marmoute> But given how similar the codepath(basically just forward
>> the boolean far down to some flag processing, I really don't see the point
>> (and things from the link Augie provided is mostly too far away from the
>> actual case here)
>> 17:59 < marmoute> remi17: So, lets drop it (unless you want to actually do
>> all that refactoring now ;-) )
And here you make a statement that’s actually covered by the blogpost, but I’m
going to re-state my position wholly in my own words on the hope that I’ll be
easier to understand (in part because I’ll use a dummy code example that’s
exactly our current case, instead of merely related):
What we’ve got as of the current series is, effectively, this:
def readstuff(pointer, do_postprocessing=True):
# read something into stuff
if do_postprocessing:
stuff = transform(stuff)
return stuff
what I’d prefer is something like this:
def readrawstuff(pointer):
# read something into stuff
return stuff
def readstuff(pointer):
stuff = readrawstuff(pointer)
return transform(stuff)
I find this appealing for (at least) three architectural reasons:
1) It makes the structural relationship between the data and the transformers
significantly more obvious
2) It makes chasing callsites of reading data or raw data a little more obvious
what the intent was
3) The extra layer will make it easier to avoid code duplication with custom
storage that’s not using revlog.
That last one, is of course, specific to this case in Mercurial, but I think
it’s an important one, especially given the direction of some of the
conversations Durham, Jun and I have been having about ways to modify changelog
storage. There’s an immediate potential consumer on the horizon!
I’m still really only comprehending an aesthetic argument from you, that you
think we don’t do raw reads enough for it to matter so you’d rather have fewer
methods, which doesn’t actually address the value proposition I enumerated
above. What do you think? Have I fundamentally misunderstood your position
somehow?
Thanks for your time.
Augie
>> 17:59 < remi17> so if you feel strongly about this, and durin42 is happy
>> with waiting for me to take a stab at it later after the censor clean up,
>> I'll proceed with raw=True
>> 17:59 < remi17> wilco
>> 18:00 < marmoute> I'm okay with processing with dropping the method and
>> using raw=True for now.
>> 18:01 < marmoute> remi17: can you drop a minimal reply to your email saying
>> that we'll drop it as per IRC discussion (so that people following the email
>> have the context)
A side-note for remi: it’d probably be good to quote the relevant IRC
conversation (or at least post an agreed-upon summary) to the mailing list so
that people who don’t lurk in IRC can still read the details of why a decision
was made.
>
> Cheers,
>
> --
> Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
