You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@xalan.apache.org by Andreas Martens1 <am...@uk.ibm.com> on 2024/09/27 13:25:11 UTC
Reindenting a file in a PR
Hello!
I’m ready to submit a PR for XALANJ-2785.
Whilst editing some of the xsltc source files I got confused because there was a mix of tabs and spaces in them.
Will I be shot for re-indenting the files to be consistent?
Cheers,
Andreas
--
Andreas Martens
[signature_558150371]
Senior Engineer
App Connect Enterprise
IBM
Phone / Signal:
+44 (0) 7824 544874
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Re: Reindenting a file in a PR
Posted by Arnout Engelen <en...@apache.org>.
On Fri, Sep 27, 2024 at 3:42 PM Andreas Martens1 <am...@uk.ibm.com>
wrote:
> Fair,
>
>
>
> It’s annoying that github can’t ignore whitespace in its diffs.
>
FWIW it can somewhat: it's under the 'cog', e.g.
https://github.com/apache/xalan-java/pull/189/files?diff=unified&w=1 .
(still, small diffs are nice even with that feature, and applying
consistent formatting in a separate commit would probably be nicer. you can
even add that commit to a '.git-blame-ignore-revs'-file which some tools
will pick up on - very useful unless something snuck in in that commit....)
Arnout
>
>
> Would you be against a separate PR that re-indents the files (at 2 or 4
> spaces, I’d prefer 2 but I understand if a project wants 4) in xsltc. When
> looking at the files as they are I struggle to follow the code!
>
>
>
> On a related note, I’m changing the handling inside xsl:element in xsltc,
> I think the changes are safe and I’m running some more tests.. Are there
> sufficient unit tests in place to have confidence I haven’t broken anything
> whilst stomping around in there?
>
>
>
> Cheers,
>
> Andreas
>
>
>
> *From: *Gary Gregory <ga...@gmail.com>
> *Date: *Friday, 27 September 2024 at 14:32
> *To: *dev@xalan.apache.org <de...@xalan.apache.org>
> *Subject: *[EXTERNAL] Re: Reindenting a file in a PR
>
> My personal POV is that PRs should be as small as possible in order to
> reduce the cognitive load on reviewers. Gary On Fri, Sep 27, 2024, 9: 27
> AM Andreas Martens1 <amartens@ uk. ibm. com> wrote: Hello! I’m ready to
> submit a PR for XALANJ-2785.
>
> My personal POV is that PRs should be as small as possible in order to
> reduce the cognitive load on reviewers.
>
>
>
> Gary
>
>
>
> On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com>
> wrote:
>
> Hello!
> I’m ready to submit a PR for XALANJ-2785.
>
> Whilst editing some of the xsltc source files I got confused because there
> was a mix of tabs and spaces in them.
>
> Will I be shot for re-indenting the files to be consistent?
>
>
>
> Cheers,
>
> Andreas
>
> --
>
> *Andreas Martens*
>
> [image: signature_558150371]
>
> Senior Engineer
>
> App Connect Enterprise
>
> IBM
>
> Phone / Signal:
>
> +44 (0) 7824 544874
>
>
>
> Unless otherwise stated above:
>
> IBM United Kingdom Limited
> Registered in England and Wales with number 741598
> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
>
> Unless otherwise stated above:
>
> IBM United Kingdom Limited
> Registered in England and Wales with number 741598
> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
>
--
Arnout Engelen
ASF Security Response
Apache Pekko PMC member, ASF Member
NixOS Committer
Independent Open Source consultant
RE: Reindenting a file in a PR
Posted by Andreas Martens1 <am...@uk.ibm.com>.
OK.
One test I cooked up was:
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl=http://www.w3.org/1999/XSL/Transform xmlns:NS1=http://www.example.com/schema/wibble/1>
<xsl:output omit-xml-declaration="yes" indent="yes"/>
<xsl:strip-space elements="*"/>
<xsl:template match="node() | @*">
<xsl:copy>
<xsl:apply-templates select="node() | @*"/>
</xsl:copy>
</xsl:template>
<xsl:template match="*[ancestor-or-self::wobble]">
<xsl:element name="{name()}" namespace=http://www.example.com/schema/wibble/0>
<xsl:apply-templates select="node() | @*"/>
</xsl:element>
</xsl:template>
<xsl:template match="*[ancestor-or-self::wubble]">
<xsl:element name="{name()}" namespace=http://www.example.com/schema/wibble/2>
<xsl:apply-templates select="node() | @*"/>
</xsl:element>
</xsl:template>
</xsl:stylesheet>
With this input:
<outer><wobble><inner><event/></inner></wobble><wubble><wobble/></wubble></outer>
Gave this output:
<outer>
<ns0:wobble xmlns:ns0=http://www.example.com/schema/wibble/0>
<ns0:inner>
<ns0:event/>
</ns0:inner>
</ns0:wobble>
<ns1:wubble xmlns:ns1=http://www.example.com/schema/wibble/2>
<ns1:wobble/>
</ns1:wubble>
</outer>
Would you call that correct? My concern is that the nested wobble has become <ns1:wobble/> from the second xsl:element and not <ns0:wobble/> as per the first.
Or is it an invalid test?
If I change the xsl to read:
<?xml version="1.0" encoding="UTF-8"?>
<xsl:stylesheet version="1.0" xmlns:xsl=http://www.w3.org/1999/XSL/Transform xmlns:NS1=http://www.example.com/schema/wibble/1>
<xsl:output omit-xml-declaration="yes" indent="yes"/>
<xsl:strip-space elements="*"/>
<xsl:template match="node() | @*">
<xsl:copy>
<xsl:apply-templates select="node() | @*"/>
</xsl:copy>
</xsl:template>
<xsl:template match="wobble">
<xsl:element name="{name()}" namespace=http://www.example.com/schema/wibble/0>
<xsl:apply-templates select="node() | @*"/>
</xsl:element>
</xsl:template>
<xsl:template match="wubble">
<xsl:element name="{name()}" namespace=http://www.example.com/schema/wibble/2>
<xsl:apply-templates select="node() | @*"/>
</xsl:element>
</xsl:template>
</xsl:stylesheet>
(i.e. remove the ancestor-or-self) I get:
<outer>
<ns0:wobble xmlns:ns0=http://www.example.com/schema/wibble/0>
<inner>
<event/>
</inner>
</ns0:wobble>
<ns1:wubble xmlns:ns1=http://www.example.com/schema/wibble/2>
<ns0:wobble xmlns:ns0=http://www.example.com/schema/wibble/0/>
</ns1:wubble>
</outer>
The declaration of the ns0 namespace appears twice now, but I *think* that’s correct, since one isn’t a descendant of the other?
*Although* I’m not sure the _inner_ element should be namespaced, if I use a different processor I get:
<outer>
<wobble xmlns=http://www.example.com/schema/wibble/0>
<inner xmlns="">
<event/>
</inner>
</wobble>
<wubble xmlns=http://www.example.com/schema/wibble/2>
<wobble xmlns=http://www.example.com/schema/wibble/0/>
</wubble>
</outer>
(i.e. it’s re-setting the anonymous namespace for the _inner_ and _event_ elements because they’re not included in the match template of xsl:element.
Alas I’m not an XSLT expert, I’m just picking up this problem because we can see one of our customers will hit it soon and I want to fix it before they do!
Cheers,
Andreas
From: Gary Gregory <ga...@gmail.com>
Date: Friday, 27 September 2024 at 14:49
To: dev@xalan.apache.org <de...@xalan.apache.org>
Subject: [EXTERNAL] Re: Reindenting a file in a PR
I think it would be best if a committer applied uniform formatting across the board. It's not a PR I would review, you just never know what little mistake could sneak in, intentionally or not. Gary On Fri, Sep 27, 2024, 9: 42 AM Andreas Martens1
I think it would be best if a committer applied uniform formatting across the board. It's not a PR I would review, you just never know what little mistake could sneak in, intentionally or not.
Gary
On Fri, Sep 27, 2024, 9:42 AM Andreas Martens1 <am...@uk.ibm.com>> wrote:
Fair,
It’s annoying that github can’t ignore whitespace in its diffs.
Would you be against a separate PR that re-indents the files (at 2 or 4 spaces, I’d prefer 2 but I understand if a project wants 4) in xsltc. When looking at the files as they are I struggle to follow the code!
On a related note, I’m changing the handling inside xsl:element in xsltc, I think the changes are safe and I’m running some more tests.. Are there sufficient unit tests in place to have confidence I haven’t broken anything whilst stomping around in there?
Cheers,
Andreas
From: Gary Gregory <ga...@gmail.com>>
Date: Friday, 27 September 2024 at 14:32
To: dev@xalan.apache.org<ma...@xalan.apache.org> <de...@xalan.apache.org>>
Subject: [EXTERNAL] Re: Reindenting a file in a PR
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers. Gary On Fri, Sep 27, 2024, 9: 27 AM Andreas Martens1 <amartens@ uk. ibm. com> wrote: Hello! I’m ready to submit a PR for XALANJ-2785.
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers.
Gary
On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com>> wrote:
Hello!
I’m ready to submit a PR for XALANJ-2785.
Whilst editing some of the xsltc source files I got confused because there was a mix of tabs and spaces in them.
Will I be shot for re-indenting the files to be consistent?
Cheers,
Andreas
--
Andreas Martens
Error! Filename not specified.
Senior Engineer
App Connect Enterprise
IBM
Phone / Signal:
+44 (0) 7824 544874
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Re: Reindenting a file in a PR
Posted by Joseph Kesselman <ke...@alum.mit.edu>.
I seem to remember that there _is_ a way to tell GitHub to ignore white space in diffs. I don't remember the syntax off hand unfortunately.
--
/_ Joe Kesselman (he/him/his)
-/ _) My Alexa skill for New Music/New Sounds fans:
/ https://www.amazon.com/dp/B09WJ3H657/
Caveat: Opinionated old geezer with overcompensated writer's block. May be redundant, verbose, prolix, sesquipedalian, didactic, officious, or redundant.
________________________________
From: Gary Gregory <ga...@gmail.com>
Sent: Friday, September 27, 2024 9:48:58 AM
To: dev@xalan.apache.org <de...@xalan.apache.org>
Subject: Re: Reindenting a file in a PR
I think it would be best if a committer applied uniform formatting across the board. It's not a PR I would review, you just never know what little mistake could sneak in, intentionally or not.
Gary
On Fri, Sep 27, 2024, 9:42 AM Andreas Martens1 <am...@uk.ibm.com>> wrote:
Fair,
It’s annoying that github can’t ignore whitespace in its diffs.
Would you be against a separate PR that re-indents the files (at 2 or 4 spaces, I’d prefer 2 but I understand if a project wants 4) in xsltc. When looking at the files as they are I struggle to follow the code!
On a related note, I’m changing the handling inside xsl:element in xsltc, I think the changes are safe and I’m running some more tests.. Are there sufficient unit tests in place to have confidence I haven’t broken anything whilst stomping around in there?
Cheers,
Andreas
From: Gary Gregory <ga...@gmail.com>>
Date: Friday, 27 September 2024 at 14:32
To: dev@xalan.apache.org<ma...@xalan.apache.org> <de...@xalan.apache.org>>
Subject: [EXTERNAL] Re: Reindenting a file in a PR
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers. Gary On Fri, Sep 27, 2024, 9: 27 AM Andreas Martens1 <amartens@ uk. ibm. com> wrote: Hello! I’m ready to submit a PR for XALANJ-2785.
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers.
Gary
On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com>> wrote:
Hello!
I’m ready to submit a PR for XALANJ-2785.
Whilst editing some of the xsltc source files I got confused because there was a mix of tabs and spaces in them.
Will I be shot for re-indenting the files to be consistent?
Cheers,
Andreas
--
Andreas Martens
[signature_558150371]
Senior Engineer
App Connect Enterprise
IBM
Phone / Signal:
+44 (0) 7824 544874
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Re: Reindenting a file in a PR
Posted by Gary Gregory <ga...@gmail.com>.
I think it would be best if a committer applied uniform formatting across
the board. It's not a PR I would review, you just never know what little
mistake could sneak in, intentionally or not.
Gary
On Fri, Sep 27, 2024, 9:42 AM Andreas Martens1 <am...@uk.ibm.com> wrote:
> Fair,
>
>
>
> It’s annoying that github can’t ignore whitespace in its diffs.
>
>
>
> Would you be against a separate PR that re-indents the files (at 2 or 4
> spaces, I’d prefer 2 but I understand if a project wants 4) in xsltc. When
> looking at the files as they are I struggle to follow the code!
>
>
>
> On a related note, I’m changing the handling inside xsl:element in xsltc,
> I think the changes are safe and I’m running some more tests.. Are there
> sufficient unit tests in place to have confidence I haven’t broken anything
> whilst stomping around in there?
>
>
>
> Cheers,
>
> Andreas
>
>
>
> *From: *Gary Gregory <ga...@gmail.com>
> *Date: *Friday, 27 September 2024 at 14:32
> *To: *dev@xalan.apache.org <de...@xalan.apache.org>
> *Subject: *[EXTERNAL] Re: Reindenting a file in a PR
>
> My personal POV is that PRs should be as small as possible in order to
> reduce the cognitive load on reviewers. Gary On Fri, Sep 27, 2024, 9: 27
> AM Andreas Martens1 <amartens@ uk. ibm. com> wrote: Hello! I’m ready to
> submit a PR for XALANJ-2785.
>
> My personal POV is that PRs should be as small as possible in order to
> reduce the cognitive load on reviewers.
>
>
>
> Gary
>
>
>
> On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com>
> wrote:
>
> Hello!
> I’m ready to submit a PR for XALANJ-2785.
>
> Whilst editing some of the xsltc source files I got confused because there
> was a mix of tabs and spaces in them.
>
> Will I be shot for re-indenting the files to be consistent?
>
>
>
> Cheers,
>
> Andreas
>
> --
>
> *Andreas Martens*
>
> [image: signature_558150371]
>
> Senior Engineer
>
> App Connect Enterprise
>
> IBM
>
> Phone / Signal:
>
> +44 (0) 7824 544874
>
>
>
> Unless otherwise stated above:
>
> IBM United Kingdom Limited
> Registered in England and Wales with number 741598
> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
>
> Unless otherwise stated above:
>
> IBM United Kingdom Limited
> Registered in England and Wales with number 741598
> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
>
RE: Reindenting a file in a PR
Posted by Andreas Martens1 <am...@uk.ibm.com>.
Fair,
It’s annoying that github can’t ignore whitespace in its diffs.
Would you be against a separate PR that re-indents the files (at 2 or 4 spaces, I’d prefer 2 but I understand if a project wants 4) in xsltc. When looking at the files as they are I struggle to follow the code!
On a related note, I’m changing the handling inside xsl:element in xsltc, I think the changes are safe and I’m running some more tests.. Are there sufficient unit tests in place to have confidence I haven’t broken anything whilst stomping around in there?
Cheers,
Andreas
From: Gary Gregory <ga...@gmail.com>
Date: Friday, 27 September 2024 at 14:32
To: dev@xalan.apache.org <de...@xalan.apache.org>
Subject: [EXTERNAL] Re: Reindenting a file in a PR
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers. Gary On Fri, Sep 27, 2024, 9: 27 AM Andreas Martens1 <amartens@ uk. ibm. com> wrote: Hello! I’m ready to submit a PR for XALANJ-2785.
My personal POV is that PRs should be as small as possible in order to reduce the cognitive load on reviewers.
Gary
On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com>> wrote:
Hello!
I’m ready to submit a PR for XALANJ-2785.
Whilst editing some of the xsltc source files I got confused because there was a mix of tabs and spaces in them.
Will I be shot for re-indenting the files to be consistent?
Cheers,
Andreas
--
Andreas Martens
[signature_558150371]
Senior Engineer
App Connect Enterprise
IBM
Phone / Signal:
+44 (0) 7824 544874
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Unless otherwise stated above:
IBM United Kingdom Limited
Registered in England and Wales with number 741598
Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
Re: Reindenting a file in a PR
Posted by Gary Gregory <ga...@gmail.com>.
My personal POV is that PRs should be as small as possible in order to
reduce the cognitive load on reviewers.
Gary
On Fri, Sep 27, 2024, 9:27 AM Andreas Martens1 <am...@uk.ibm.com> wrote:
> Hello!
> I’m ready to submit a PR for XALANJ-2785.
>
> Whilst editing some of the xsltc source files I got confused because there
> was a mix of tabs and spaces in them.
>
> Will I be shot for re-indenting the files to be consistent?
>
>
>
> Cheers,
>
> Andreas
>
> --
>
> *Andreas Martens*
>
> [image: signature_558150371]
>
> Senior Engineer
>
> App Connect Enterprise
>
> IBM
>
> Phone / Signal:
>
> +44 (0) 7824 544874
>
>
> Unless otherwise stated above:
>
> IBM United Kingdom Limited
> Registered in England and Wales with number 741598
> Registered office: PO Box 41, North Harbour, Portsmouth, Hants. PO6 3AU
>