public inbox for devel@edk2.groups.io
 help / color / mirror / Atom feed
From: "Leif Lindholm" <quic_llindhol@quicinc.com>
To: Michael Kubacki <mikuback@linux.microsoft.com>
Cc: <devel@edk2.groups.io>, <ardb@kernel.org>,
	Ard Biesheuvel <ardb+tianocore@kernel.org>,
	Sami Mujawar <sami.mujawar@arm.com>,
	"Kinney, Michael D" <michael.d.kinney@intel.com>
Subject: Re: [edk2-devel] [PATCH v3 13/14] ArmPkg: Turn off spellcheck audit mode
Date: Thu, 15 Dec 2022 10:42:51 +0000	[thread overview]
Message-ID: <Y5r6K3OT8EZV4CB9@qc-i7.hemma.eciton.net> (raw)
In-Reply-To: <910eccca-b219-86ae-5550-339650b69461@linux.microsoft.com>

On Wed, Dec 14, 2022 at 19:04:06 -0500, Michael Kubacki wrote:
> I'm just trying to understand your position.
> 
> Are you saying you would rather people check in typos and then later have
> patches come into the package to fix them?
> 
> For example, like these:
> 
> - ArmVirtPkg: https://edk2.groups.io/g/devel/message/97021
> - ArmPkg: https://edk2.groups.io/g/devel/message/97022
> 
> Why not just have the code checked in without typos in the first place?

A little fairy once whispered in my ear that if I stopped myself and
tried to rephrase whenever I found myself using the work "just", I
would meet less friction in context-stripping communication mediums
such as email. They weren't wrong.

> Checking in typos creates more review work, makes the code history have typo
> fix patches that could be avoided, and impacts accessibility.
> 
> I spend far more time with edk2 overhead such as email formatting problems,
> keeping track of maintainer email address changes when updating patches,
> mapping email replies back to code, and so on that does not improve the
> code. A spell checker only takes seconds and is built into edk2 CI.

We didn't disable the spellchecker for the ARM* packages (only)
because we hate correct spelling. We disabled it because it throws
false positives left right and centre. So we end up needing to update
the .ci.yaml every time we mention an architectural concept we haven't
mentioned before. Or add a copyright line mentioning a new
organisation. Or correctly use apostrophes in ways the spellchecker
doesn't expect.

Here is the current (and very incomplete, oh - and package specific)
exception list for ArmPkg:

https://github.com/tianocore/edk2/blob/master/ArmPkg/ArmPkg.ci.yaml#L95

If it had a aggressiveness knob and we could turn it down from 11 to
3 and work our way up from there, that would be another story.

Failing that, a push-through-anyway-this-isn't-a-typo label would be a
reasonable compromise.

But for now, I agree with Ard.

/
    Leif

> On 12/14/2022 6:24 PM, Ard Biesheuvel wrote:
> > On Thu, 15 Dec 2022 at 00:21, Michael Kubacki
> > <mikuback@linux.microsoft.com> wrote:
> > > 
> > > Yes. It will also reduce frequency of incoming patches that must be
> > > reviewed and merged due to people continuously fixing trivial spelling
> > > errors.
> > > 
> > 
> > In that case, NAK to this patch (and the ArmVirtPkg one). Unless we
> > add a button to the GitHub UI that permits me to override a negative
> > CI result on a PR.
> > 
> > > On 12/14/2022 6:07 PM, Ard Biesheuvel wrote:
> > > > On Wed, 14 Dec 2022 at 23:53, <mikuback@linux.microsoft.com> wrote:
> > > > > 
> > > > > From: Michael Kubacki <michael.kubacki@microsoft.com>
> > > > > 
> > > > > Audit mode was enabled for the spellcheck CI plugin. It is no longer
> > > > > needed with recent changes. Spelling errors can be checked in the
> > > > > package moving forward.
> > > > > 
> > > > > Cc: Leif Lindholm <quic_llindhol@quicinc.com>
> > > > > Cc: Ard Biesheuvel <ardb+tianocore@kernel.org>
> > > > > Cc: Sami Mujawar <sami.mujawar@arm.com>
> > > > > Signed-off-by: Michael Kubacki <michael.kubacki@microsoft.com>
> > > > 
> > > > Will this patch result in PRs potentially being rejected by pre-merge
> > > > CI due to trivial spelling errors?
> > > > 
> > > > 
> > > > > ---
> > > > >    ArmPkg/ArmPkg.ci.yaml | 3 ++-
> > > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/ArmPkg/ArmPkg.ci.yaml b/ArmPkg/ArmPkg.ci.yaml
> > > > > index c8dface6821a..a304c7966cf7 100644
> > > > > --- a/ArmPkg/ArmPkg.ci.yaml
> > > > > +++ b/ArmPkg/ArmPkg.ci.yaml
> > > > > @@ -87,7 +87,7 @@
> > > > > 
> > > > >        ## options defined .pytool/Plugin/SpellCheck
> > > > >        "SpellCheck": {
> > > > > -        "AuditOnly": True,
> > > > > +        "AuditOnly": False,
> > > > >            "IgnoreFiles": [
> > > > >                "Library/ArmSoftFloatLib/berkeley-softfloat-3/**"
> > > > >            ],                           # use gitignore syntax to ignore errors
> > > > > @@ -148,6 +148,7 @@
> > > > >              "fcmplt",
> > > > >              "ffreestanding",
> > > > >              "frsub",
> > > > > +          "hauser",
> > > > >              "hisilicon",
> > > > >              "iccabpr",
> > > > >              "iccbpr",
> > > > > --
> > > > > 2.28.0.windows.1
> > > > > 
> > 
> > 
> > 
> > 

  parent reply	other threads:[~2022-12-15 10:43 UTC|newest]

Thread overview: 36+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-12-14 22:52 [PATCH v3 00/14] Fix new typos reported Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 01/14] PrmPkg: " Michael Kubacki
2022-12-20  3:09   ` [edk2-devel] " Ankit Sinha
2022-12-20 22:19     ` Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 02/14] StandaloneMmPkg: " Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 03/14] DynamicTablesPkg: " Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 04/14] UnitTestFrameworkPkg: " Michael Kubacki
2022-12-14 23:05   ` Michael D Kinney
2022-12-14 22:52 ` [PATCH v3 05/14] FatPkg: " Michael Kubacki
2022-12-14 23:07   ` [edk2-devel] " Michael D Kinney
2022-12-14 22:52 ` [PATCH v3 06/14] FmpDevicePkg: " Michael Kubacki
2022-12-14 23:05   ` Michael D Kinney
2022-12-14 22:52 ` [PATCH v3 07/14] ArmPkg: Ignore " Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 08/14] ArmVirtPkg: Add new ignored spelling errors Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 09/14] ArmPkg: Fix typos Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 10/14] ArmPlatformPkg: " Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 11/14] ArmVirtPkg: " Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 12/14] .azurepipelines: Update cspell version to 5.21.0 Michael Kubacki
2022-12-14 23:06   ` [edk2-devel] " Michael D Kinney
2022-12-14 22:52 ` [PATCH v3 13/14] ArmPkg: Turn off spellcheck audit mode Michael Kubacki
2022-12-14 23:07   ` Ard Biesheuvel
2022-12-14 23:21     ` Michael Kubacki
2022-12-14 23:24       ` Ard Biesheuvel
2022-12-15  0:04         ` [edk2-devel] " Michael Kubacki
2022-12-15  8:46           ` Ard Biesheuvel
2022-12-15 10:42           ` Leif Lindholm [this message]
2022-12-15 16:38             ` Michael Kubacki
2022-12-15 16:57               ` Michael D Kinney
2022-12-16 17:36                 ` Michael Kubacki
2023-01-04 10:37               ` Ard Biesheuvel
2023-01-06  2:46                 ` Michael Kubacki
2023-01-11 16:23                   ` Ard Biesheuvel
2023-01-13  1:50                     ` Michael Kubacki
2022-12-14 22:52 ` [PATCH v3 14/14] ArmVirtPkg: " Michael Kubacki
2022-12-15 16:40 ` [PATCH v3 00/14] Fix new typos reported Michael Kubacki
2022-12-16  2:35   ` Dongdong Zhang

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-list from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=Y5r6K3OT8EZV4CB9@qc-i7.hemma.eciton.net \
    --to=devel@edk2.groups.io \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox