CodingStyle on existing code

CodingStyle on existing code

[Wido den Hollander]

Hi,

While working on mon/PGMonitor.cc I see that there is a lot of
inconsistency on the code.

A lot of whitespaces, indentation which is not correct, well, a lot of
things.

Is this something we want to fix? With some scripts we can probably do
this easily, but it might cause merge hell with people working on features.

Wido

Re: CodingStyle on existing code

[Loic Dachary]

[-- Attachment #1: Type: text/plain, Size: 833 bytes --]



On 01/12/2015 14:10, Wido den Hollander wrote:
> Hi,
> 
> While working on mon/PGMonitor.cc I see that there is a lot of
> inconsistency on the code.
> 
> A lot of whitespaces, indentation which is not correct, well, a lot of
> things.
> 
> Is this something we want to fix? With some scripts we can probably do
> this easily, but it might cause merge hell with people working on features.

A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)

My 2cts.

> Wido
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

-- 
Loïc Dachary, Artisan Logiciel Libre


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: CodingStyle on existing code

[Gregory Farnum]

On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
>
>
> On 01/12/2015 14:10, Wido den Hollander wrote:
>> Hi,
>>
>> While working on mon/PGMonitor.cc I see that there is a lot of
>> inconsistency on the code.
>>
>> A lot of whitespaces, indentation which is not correct, well, a lot of
>> things.
>>
>> Is this something we want to fix? With some scripts we can probably do
>> this easily, but it might cause merge hell with people working on features.
>
> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)

Yeah, we generally want you to follow the standards in any new code. A
mass update of the code style on existing code makes navigating the
history a little harder so a lot of people don't like it much, though.
*shrug*
-Greg

Re: CodingStyle on existing code

[Wido den Hollander]

On 01-12-15 16:00, Gregory Farnum wrote:
> On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
>>
>>
>> On 01/12/2015 14:10, Wido den Hollander wrote:
>>> Hi,
>>>
>>> While working on mon/PGMonitor.cc I see that there is a lot of
>>> inconsistency on the code.
>>>
>>> A lot of whitespaces, indentation which is not correct, well, a lot of
>>> things.
>>>
>>> Is this something we want to fix? With some scripts we can probably do
>>> this easily, but it might cause merge hell with people working on features.
>>
>> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)
> 
> Yeah, we generally want you to follow the standards in any new code. A
> mass update of the code style on existing code makes navigating the
> history a little harder so a lot of people don't like it much, though.

Understood. But in this case I'm working in PGMonitor.cc. For just 20
lines of code I probably shouldn't refactor the whole file, should I?

> *shrug*
> -Greg
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: CodingStyle on existing code

[Sage Weil]

On Tue, 1 Dec 2015, Wido den Hollander wrote:
> 
> On 01-12-15 16:00, Gregory Farnum wrote:
> > On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
> >>
> >>
> >> On 01/12/2015 14:10, Wido den Hollander wrote:
> >>> Hi,
> >>>
> >>> While working on mon/PGMonitor.cc I see that there is a lot of
> >>> inconsistency on the code.
> >>>
> >>> A lot of whitespaces, indentation which is not correct, well, a lot of
> >>> things.
> >>>
> >>> Is this something we want to fix? With some scripts we can probably do
> >>> this easily, but it might cause merge hell with people working on features.
> >>
> >> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)
> > 
> > Yeah, we generally want you to follow the standards in any new code. A
> > mass update of the code style on existing code makes navigating the
> > history a little harder so a lot of people don't like it much, though.
> 
> Understood. But in this case I'm working in PGMonitor.cc. For just 20
> lines of code I probably shouldn't refactor the whole file, should I?

Easiest thing is to fix the code around your change.

I'm also open to a wholesale cleanup since it's a low-traffic file and 
likely won't conflict with other stuff in flight.  But, up to you!

sage

Re: CodingStyle on existing code

[Joao Eduardo Luis]

On 12/01/2015 03:18 PM, Sage Weil wrote:
> On Tue, 1 Dec 2015, Wido den Hollander wrote:
>>
>> On 01-12-15 16:00, Gregory Farnum wrote:
>>> On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>
>>>>
>>>> On 01/12/2015 14:10, Wido den Hollander wrote:
>>>>> Hi,
>>>>>
>>>>> While working on mon/PGMonitor.cc I see that there is a lot of
>>>>> inconsistency on the code.
>>>>>
>>>>> A lot of whitespaces, indentation which is not correct, well, a lot of
>>>>> things.
>>>>>
>>>>> Is this something we want to fix? With some scripts we can probably do
>>>>> this easily, but it might cause merge hell with people working on features.
>>>>
>>>> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)
>>>
>>> Yeah, we generally want you to follow the standards in any new code. A
>>> mass update of the code style on existing code makes navigating the
>>> history a little harder so a lot of people don't like it much, though.
>>
>> Understood. But in this case I'm working in PGMonitor.cc. For just 20
>> lines of code I probably shouldn't refactor the whole file, should I?

While it annoys me finding a given commit in history that just changes
every single line to fix styling, I also recognize that this is the sort
of janitorial task that may be warranted for certain files.

As sage mentions below, this is a low-traffic file. And whenever it's
changed, is often just tiny bits here and there. That tends to add to
the style divergence rather than to convergence.

I'd say go for it. If you are indeed changing those 20 or so lines
though, please add those lines on a separate patch from the style changes.

  -Joao

> 
> Easiest thing is to fix the code around your change.
> 
> I'm also open to a wholesale cleanup since it's a low-traffic file and 
> likely won't conflict with other stuff in flight.  But, up to you!
> 
> sage
> --
> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

Re: CodingStyle on existing code

[Wido den Hollander]

On 12/03/2015 12:12 PM, Joao Eduardo Luis wrote:
> On 12/01/2015 03:18 PM, Sage Weil wrote:
>> On Tue, 1 Dec 2015, Wido den Hollander wrote:
>>>
>>> On 01-12-15 16:00, Gregory Farnum wrote:
>>>> On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>>
>>>>>
>>>>> On 01/12/2015 14:10, Wido den Hollander wrote:
>>>>>> Hi,
>>>>>>
>>>>>> While working on mon/PGMonitor.cc I see that there is a lot of
>>>>>> inconsistency on the code.
>>>>>>
>>>>>> A lot of whitespaces, indentation which is not correct, well, a lot of
>>>>>> things.
>>>>>>
>>>>>> Is this something we want to fix? With some scripts we can probably do
>>>>>> this easily, but it might cause merge hell with people working on features.
>>>>>
>>>>> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)
>>>>
>>>> Yeah, we generally want you to follow the standards in any new code. A
>>>> mass update of the code style on existing code makes navigating the
>>>> history a little harder so a lot of people don't like it much, though.
>>>
>>> Understood. But in this case I'm working in PGMonitor.cc. For just 20
>>> lines of code I probably shouldn't refactor the whole file, should I?
> 
> While it annoys me finding a given commit in history that just changes
> every single line to fix styling, I also recognize that this is the sort
> of janitorial task that may be warranted for certain files.
> 
> As sage mentions below, this is a low-traffic file. And whenever it's
> changed, is often just tiny bits here and there. That tends to add to
> the style divergence rather than to convergence.
> 
> I'd say go for it. If you are indeed changing those 20 or so lines
> though, please add those lines on a separate patch from the style changes.
> 

Here is the first pull request without a functional change. It is just
the codingstyle fix: https://github.com/ceph/ceph/pull/6881

If that comes through I can put in the actual code.

Wido

>   -Joao
> 
>>
>> Easiest thing is to fix the code around your change.
>>
>> I'm also open to a wholesale cleanup since it's a low-traffic file and 
>> likely won't conflict with other stuff in flight.  But, up to you!
>>
>> sage
>> --
>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>
> 


-- 
Wido den Hollander
42on B.V.
Ceph trainer and consultant

Phone: +31 (0)20 700 9902
Skype: contact42on

Re: CodingStyle on existing code

[Joao Eduardo Luis]

On 12/09/2015 11:16 PM, Wido den Hollander wrote:
> On 12/03/2015 12:12 PM, Joao Eduardo Luis wrote:
>> On 12/01/2015 03:18 PM, Sage Weil wrote:
>>> On Tue, 1 Dec 2015, Wido den Hollander wrote:
>>>>
>>>> On 01-12-15 16:00, Gregory Farnum wrote:
>>>>> On Tue, Dec 1, 2015 at 5:47 AM, Loic Dachary <loic@dachary.org> wrote:
>>>>>>
>>>>>>
>>>>>> On 01/12/2015 14:10, Wido den Hollander wrote:
>>>>>>> Hi,
>>>>>>>
>>>>>>> While working on mon/PGMonitor.cc I see that there is a lot of
>>>>>>> inconsistency on the code.
>>>>>>>
>>>>>>> A lot of whitespaces, indentation which is not correct, well, a lot of
>>>>>>> things.
>>>>>>>
>>>>>>> Is this something we want to fix? With some scripts we can probably do
>>>>>>> this easily, but it might cause merge hell with people working on features.
>>>>>>
>>>>>> A sane (but long) way to do that is to cleanup when fixing a bug or adding a feature. With (a lot) of patience, it will eventually be better :-)
>>>>>
>>>>> Yeah, we generally want you to follow the standards in any new code. A
>>>>> mass update of the code style on existing code makes navigating the
>>>>> history a little harder so a lot of people don't like it much, though.
>>>>
>>>> Understood. But in this case I'm working in PGMonitor.cc. For just 20
>>>> lines of code I probably shouldn't refactor the whole file, should I?
>>
>> While it annoys me finding a given commit in history that just changes
>> every single line to fix styling, I also recognize that this is the sort
>> of janitorial task that may be warranted for certain files.
>>
>> As sage mentions below, this is a low-traffic file. And whenever it's
>> changed, is often just tiny bits here and there. That tends to add to
>> the style divergence rather than to convergence.
>>
>> I'd say go for it. If you are indeed changing those 20 or so lines
>> though, please add those lines on a separate patch from the style changes.
>>
> 
> Here is the first pull request without a functional change. It is just
> the codingstyle fix: https://github.com/ceph/ceph/pull/6881
> 
> If that comes through I can put in the actual code.

Seems a whole lot less intrusive than I expected. :)

  -Joao

> 
> Wido
> 
>>   -Joao
>>
>>>
>>> Easiest thing is to fix the code around your change.
>>>
>>> I'm also open to a wholesale cleanup since it's a low-traffic file and 
>>> likely won't conflict with other stuff in flight.  But, up to you!
>>>
>>> sage
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe ceph-devel" in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>>>
>>
> 
>