As noted in the patch, this BZ was filed to follow up and review those:
https://bugzilla.tianocore.org/show_bug.cgi?id=3929
I don't like doing this either but the spelling errors do exist. I am
trying not to make CI policy changes as those can be controversial even
among maintainers in the same package and is an orthogonal conversation
to addressing pre-existing issues within the presently defined CI policy.
In this specific case, the ignore list in the package CI YAML file can
be used to explicitly identify known typos and the BZ explicitly tracks
reviewing those so there's a well defined path to resolve and fix the issue.
I personally feel that's better than ignoring the problem entirely but
it also depends on where your package code ends up getting consumed and
the requirements and burden it might place on those consumers. For
example, if it ends up in auto generated documentation and that
documentation has spell check enabled, it becomes a downstream override.
There's currently several PRs active that fix typos so others see some
value in this work (as opposed to disabling spell checking):
- https://github.com/tianocore/edk2/pull/2900
- https://github.com/tianocore/edk2/pull/2789
- https://github.com/tianocore/edk2/pull/1955
For future changes, I suggested lock the cspell version and I think
that's an option to prevent these from appearing at unknown points in
time. I'm not appointed to make authoritative decisions about that (to
my understanding) so I am making that suggestion for the community to
consider.
Again, I don't have a strong opinion on this topic, I've been waiting 4
weeks to get the v5 patch series merged (other busy work in between),
and you're the maintainer. It sounds like if I take ownership of BZ
3929, you might be okay with leaving it enabled? I can do that but
there's so many words in this instance, I wanted someone closer to the
package contents to look at it.
If you still strongly feel you would prefer to have it disabled, I will
pull that change in and see if any opposing opinions surface. However, I
wanted to double check this is what you want to do right now.
Thanks,
Michael
On 5/17/2022 1:31 PM, Ard Biesheuvel wrote:
On Tue, 17 May 2022 at 18:25, Michael Kubacki
<mikub...@linux.microsoft.com> wrote:
Hi Ard,
I think that's a reasonable approach.
We could also consider locking onto a specific cspell version to
decrease the likelihood of this sporadically appearing in the future.
In this case, I would prefer not to make the decision to disable spell
check entirely on behalf of various package maintainers though. I'm just
trying to keep the status quo from unblocking other changes.
Do you think that's something you or others could add as a change on top
of this series?
I guess that could wait until after the stable tag. However, your
current series adds words such as "addressig" and "characteritics" to
the ignorelist of ArmPkg, and that is something I do have a problem
with: this is just busywork, as there is no confusion whatsoever about
the meaning of the texts in question, nobody is proposing changes to
the code that contains these typos, and adding typos to these
ignorelists is clearly not the correct solution.
CI and automation are fine, but here, they are just creating problems,
resulting in pointless churn, and impeding the stable tag process.
Please, just turn it off.
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89853): https://edk2.groups.io/g/devel/message/89853
Mute This Topic: https://groups.io/mt/91166946/21656
Group Owner: devel+ow...@edk2.groups.io
Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com]
-=-=-=-=-=-=-=-=-=-=-=-
