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).
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
_______________________________________________
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
