kicad-developers team mailing list archive
Mailing list archive
Re: Pcbnew display origin transforms for v6
On 5/6/2019 5:11 PM, John Beard wrote:
> On 06/05/2019 21:46, Wayne Stambaugh wrote:
>> On 5/6/19 4:09 PM, John Beard wrote:
>>> On 06/05/2019 17:51, Reece R. Pollack wrote:
>>>> I've already jumped to clang-format 6.0, which is one of the optional
>>>> installs for Mint 18. That works, once you get all the symlinks fixed,
>>> Good to know, thanks for the update.
>>>> except it keeps wanting to reformat my switch statements like this,
>>>> which is contrary to the KiCad coding standards:
>>>> @@ -148,15 +130,9 @@ int PCB_ORIGIN_TRANSFORM_Y_ABS::FromDisplay( int
>>>> aValue ) const
>>>> case ORIGIN_REFERENCE_PAGE:
>>>> // No-op
>>>> - case ORIGIN_REFERENCE_AUX:
>>>> - origin = m_PcbBaseFrame->GetAuxOrigin().y;
>>>> - break;
>>>> - case ORIGIN_REFERENCE_GRID:
>>>> - origin = m_PcbBaseFrame->GetGridOrigin().y;
>>>> - break;
>>>> - default:
>>>> - wxASSERT(false);
>>>> - break;
>>>> + case ORIGIN_REFERENCE_AUX: origin =
>>>> m_PcbBaseFrame->GetAuxOrigin().y; break;
>>>> + case ORIGIN_REFERENCE_GRID: origin =
>>>> m_PcbBaseFrame->GetGridOrigin().y; break;
>>>> + default: wxASSERT( false ); break;
>>>> // Invert the direction if needed
>>> This is a weird style that I don't personally like, and IMO goes against
>>> the style implied by our "spacious" bracing and newline-before-if
>>> policies. But there are quite some uses of it. It's enforced by this
>>> AllowShortCaseLabelsOnASingleLine: true
>>> It's the only line-condensing option we have set:
>>> AllowShortBlocksOnASingleLine: false
>>> AllowShortCaseLabelsOnASingleLine: true # the only true here
>>> AllowShortFunctionsOnASingleLine: false
>>> AllowShortIfStatementsOnASingleLine: false
>>> AllowShortLoopsOnASingleLine: false
>>> If the formatter is proposing a change that goes against the existing
>>> style in the code area in question, I do not think it's controversial to
>>> ignore its suggestion.
>>> @Wayne, what do you think: is this enforcement representative of the
>>> right style? Should we change AllowShortCaseLabelsOnASingleLine to
>>> false? (+1 from me).
>> I would rather not have clang-format force this change. I am OK if the
>> developer prefers this for short case labels but if they prefer a
>> separate line that's fine. I prefer everything on more than one line
>> but if I'm changing code that is already formatted on a single line, I
>> don't change it. We didn't specify this in the coding policy so it's a
>> don't care but I would ask you to change it if you your case labels were
>> complex and you had them on a single line.
> Fair enough - gratuitous formatting changes in the same commit as
> functional changes are a sin anyway!
> Remember, (git-)clang-format will only change what you change, it
> shouldn't forcibly change formatting in general unless you tell it to.
> Users still need to ensure it looks sane and only accept good changes.
> Having AllowShortCaseLabelsOnASingleLine = true means that clang-format
> will *enforce* the all-on-a-line policy when it can, but it will not
> enforce column alignment (and will actually collapse manual column
> alignment). So basically, the formatter is unlikely to ever give a good
> result for a short-case switch as it stands.
> I still suggest to change it to false be default and allow developers to
> manually align when they want (and then override the formatter). Then at
> least the default behaviour is a valid formatting choice.
That works for me.