Open Stack

On 08/18/2014 06:06 PM, Daniel P. Berrange wrote:
> On Mon, Aug 18, 2014 at 11:27:39AM -0400, Doug Hellmann wrote:
>>
>> On Aug 18, 2014, at 10:15 AM, Daniel P. Berrange <berrange at redhat.com> wrote:
>>
>>> On Mon, Aug 18, 2014 at 07:57:28AM +1000, Michael Still wrote:
>>>> My recollection is that this was a request from the oslo team, but it
>>>> was so long ago that I don't recall the details.
>>>>
>>>> I think the change is low value, so should only be done when someone
>>>> is changing the logging in a file already (the log hinting for
>>>> example).
>>>
>>> The "lazy conversion" approach really encourages bad practice and is very
>>> wasteful for developers/reviewers. In GIT commit guidelines we explicitly
>>> say not to make code cleanups in their code that are unrelated to the
>>> feature/bug being addressed. When we have done lazy conversion for this
>>> kind of thing, I've seen it waste a hell of alot of time for developers.
>>> People are never entirely clear which is the preferred style, so they
>>> end up just making a guess which will often be wrong. So now we consume
>>> scarce reviewer time pointing this out to people over & over & over again,
>>> and waste developer time having them re-post their patches again.
>>>
>>> If we want to change LOG.warning to LOG.warn, we should do a single
>>> patch with a global search & replace to get the pain over & done with
>>> as soon as possible, then enforce it with a hacking rule. No reviewer
>>> time gets wasted and developers will see their mistake right away.
>>
>> The only issue I can see with that approach is causing existing patches
>> to have to be rebased. That can be mitigated by making these sorts of
>> cleanups at a time when other feature changes aren’t going to be under
>> development, though.
> 
> The pessimist in me says that the majority of existing patches are going
> have to be rebased many more times for unrelated reasons already, so this
> won't be as bad as you might fear. It would make sense to avoid doing
> these kind of cleanups immediately before any release milestones though,
> since there is no sense in intentionally inflicting this pain at the
> worst possible time :-)
> 
> So IMHO there is a tiny window for someone to propose this for Juno
> right now if reviewers commit to merging it quickly. Otherwise any
> conversion should wait until Kilo opens, and don't attempt to do a
> piece-by-piece conversion in the meantime.

I agree with Daniel here. I'd rather have a single, ninja-approved patch
changing all matches than several small patches that introduce unrelated
changes to replace warn with warning. An additional benefit is that the
single-replacement patch may also be used as a reference for future
commits using `warn` instead of `warning`.

My $0.02,
Flavio


-- 
@flaper87
Flavio Percoco