Hi Ard,
I understand it is frustrating for things that were working to suddenly
stop and errors to have been missed by the plugin in the past. I'm also
surprised that some of these issues were previously not caught.
To clarify, adding the words to the ignore list was not really that much
time. The plugin output gives the words to add to the list (in JSON) so
that's a copy/paste operation and an IDE can remove duplicate lines
instantly so that was about a 10-30 second or so solution. Submitting
the BZ was another 1-2 minutes
Following the the edk2 contribution process to manually add maintainers
per package, rebase and manually add review tags, parse feedback inline
to unified diffs over email, generate patch files, and update the cover
letter was a relatively larger consumer of time. For v2, I took
ownership of the BZ and spent more time to try to reduce the likelihood
of unexpected issues appearing in the future.
V2 will do the following:
1. Complete BZ 3929.
2. Lock the cspell version to v5.20.0 to prevent latest from
unexpectedly causing issues in the future.
3. Update the common word list in cspell.base.yaml to prevent package
level duplication in the future.
4. Include Sami's code review tags.
I'm checking the CI results in the PR now and once it passes, I'll send
it on the list.
https://github.com/tianocore/edk2/pull/2903
Thanks,
Michael
On 5/17/2022 4:06 PM, Ard Biesheuvel wrote:
On Tue, 17 May 2022 at 21:32, Michael Kubacki
<mikub...@linux.microsoft.com> wrote:
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.
If you feel it is worth your time to fix typos in existing comments, I
won't stand in your way. But I don't feel it is worth my time, given
that it doesn't actually improve the code, except for by some
artifical measure of spelling-correctness, which has no bearing at all
on what runs on people's machines, and as far as I can tell, these
typoes do not create any confusion regarding what the comments intend
to convey.
Adding typoed words to the ignorelist is the worst possible solution,
because you will be wasting your time only to placate the machine,
accumulating technical debt in the code base without actually fixing
the problems. So that is out of the question for me.
If you want to fix these issues, that is also fine. I will review/ack
with priority provided that I actually have any bandwidth available.
But if we are working for the CI instead of the other way around,
something is seriously wrong. If we can't roll a stable tag because
the CI wants us to fix our typoes first, we have to be able to
override it. And corrupting the codebase by adding typoes to the
ignorelist just to placate the CI is preposterous..
-=-=-=-=-=-=-=-=-=-=-=-
Groups.io Links: You receive all messages sent to this group.
View/Reply Online (#89855): https://edk2.groups.io/g/devel/message/89855
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]
-=-=-=-=-=-=-=-=-=-=-=-
