lint: Make lint spellcheck a failure instead of warning #25706

1. 
   aureleoules commented at 9:58 am on July 26, 2022: member

   This change makes the spellcheck test a failure instead of warning when it fails. See #25701 (comment).

   I have also refactored the test to address MarcoFalke’s comments #24766#pullrequestreview-932953476.

2. fanquake added the label Scripts and tools on Jul 26, 2022
3. lint: Make lint spellcheck a failure instead of warning fefbca3509
4. aureleoules force-pushed on Jul 26, 2022
5. 
   shaavan commented at 11:47 am on July 26, 2022: contributor

   ACK fefbca3509781740d606c1b43588c07aa0a36dae

   • I agree with the concept of making spellcheck be an error instead of a warning.
   • Though, at first, I thought it was too strict for a spelling mistake to be deemed an error, I understand that this will save a lot of maintenance burden and remove the need for PRs to fix spellings.
   • I verified the code and confirmed all the suggestions from @MarcoFalke’s this comment have been tackled here.
   • I was able to verify the success working of this PR by using the following patch

   0 #include <util/time.h>
   1-
   2+// this is a test coment
   3 #include <deque>
   4 #include <functional>
   

   and getting the following error:

   0src/torcontrol.cpp:21: coment ==> comment
   1^ Error: codespell identified likely spelling errors. Any false positives? Add them to the list of ignored words in test/lint/spelling.ignore-words.txt
   

6. 
   jarolrod commented at 8:18 pm on July 26, 2022: member

   concept ack
7. 
   jarolrod commented at 6:03 am on August 2, 2022: member

   Thought about this a little more, I’m concept 0 on this.

   Incorrect spelling of words in an inconsequential change shouldn’t lead to the CI being red.

   What if your first language is not English, and you just care about writing good code? You publish a change, and CI keeps failing because of these darn English words; no one is leaving a review to fix your spelling or grammar. So your PR sits, where otherwise it might have been merged. This can be discouraging for new developers; especially of different backgrounds/native languages.

   Now, in consideration of a really important change, and the comments written along with it, {insert criteria of important change here}, this would be more appropriate. But, we can’t selectively turn this on or off.

   If we enable this, there should be a commitment to at least address the first scenario, if it ever is to come up. But, I’d rather avoid polluting PR’s with grammar nazism.

   Concept ACK on the changes outside of making this a failure :)

8. 
   aureleoules commented at 3:09 pm on August 3, 2022: member

   @jarolrod I understand your point but isn’t it a bit extreme? Also, wouldn’t you already need to have a decent english level to start contributing to Bitcoin as most of the implementation documentation / code is written in english?

   But, I’d rather avoid polluting PR’s with grammar nazism.

   I understand but this change would also avoid pull requests that fix these mistakes.

   It would be great to have a yellow/warning status for GitHub checks but unfortunately GitHub doesn’t provide this feature. I’m happy to drop the exit(1) if others feel the same.

9. 
   DrahtBot commented at 10:24 am on September 23, 2022: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Conflicts

   No conflicts as of last run.

10. achow101 closed this on Oct 12, 2022

    ---

11. 
    Empact commented at 7:53 pm on October 12, 2022: member

    Post-close Concept NACK - IMO while it would be nice to give more visibility to these issues (e.g. via a comment to github?), failing CI is out of scale to the problem and can interfere with the iteration pace of development, particularly given the possibility of false positives.
12. aureleoules deleted the branch on Nov 2, 2022
13. bitcoin locked this on Nov 2, 2023

Contributors
aureleoules shaavan jarolrod DrahtBot Empact

Labels
Scripts and tools