[PATCH 2 of 5 flagprocessor v6] revlog: add 'raw' argument to revision and _addrevision

Pierre-Yves David pierre-yves.david at ens-lyon.org
Sat Jan 7 16:39:29 UTC 2017



On 12/30/2016 08:46 PM, Augie Fackler wrote:
>
> On Dec 30, 2016 12:30, "Pierre-Yves David" <pierre-yves.david at 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.chaintron at 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).

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.


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.
> 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 ;-) )
> 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)

Cheers,

-- 
Pierre-Yves David


More information about the Mercurial-devel mailing list