Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2873324787 I am fine with either, but the rule require at least two member approve to merge your change. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318:
URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2873226277
> why not fix the spell error file by file?
I tried to explain it before.
Sorry - I do not want to sound rude. And I do not want to the waste the
boring labor of two long evenings, which went into preparing the remaining
patches (waiting here locally).
# Solution Proposal I
It seems, that I misunderstood the severity of the "all checks must pass"
requirement of the nuttx community. In this case I would ask to revert my
previous commit ("ci: enable spellchecks",
7088d2ee91fa8019fc8e1772a3c8c84a3bd59ba7). This will allow me to submit my
remaining commits without investing even more evenings of rather boring work
into rearranging them.
# Solution Proposal II
I will prepare a PR containing the five(?) remaining commits and I will
leave it to you (or other members of the community) to re-arrange this into an
order that satisfies the requirements. As a reminder: the biggest commit has
this volume:
> 1719 files changed, 3243 insertions(+), 3071 deletions(-)
Thus, the decision of taking over this work should not be taken lightly.
Which approach would you prefer?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle closed pull request #16318: Documentation: fix spelling URL: https://github.com/apache/nuttx/pull/16318 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2870575447 > > > I would like to wrap this PR up. Is there anything missing? > > > > > > still has error not fix yet: https://github.com/apache/nuttx/actions/runs/14902428691/job/41857195862?pr=16318 https://github.com/apache/nuttx/actions/runs/14902428711/job/41857195812?pr=16318 > > These remaining issues were beyound my understanding of the original author's intention. Thus, I cecided to refrain from guessing (and potentially breaking) their meaning. > > I think, [my comment from #16319](https://github.com/apache/nuttx/pull/16319#issuecomment-2870029622) applies here, as well: I think, I prepared the spelling fixes commits with a good amount of care and diligence. But I do not think, we should burden my commits for "fixing 99% of spelling issues" with the task of really fixing 100% of the issues, just because I introduced the new CI checks _before_ all of my commits were merged. why not fix the spell error file by file? so you can group the reasonable number of files in one patch and fix all spelling problem in them. All members include PMC have to follow the guide enforce by the community, which require: 1. Pass all CI check 2. the breaking change need be voted and passed in community 3. At least two people approve the change -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2870190208 > > I would like to wrap this PR up. Is there anything missing? > > still has error not fix yet: https://github.com/apache/nuttx/actions/runs/14902428691/job/41857195862?pr=16318 https://github.com/apache/nuttx/actions/runs/14902428711/job/41857195812?pr=16318 These remaining issues were beyound my understanding of the original author's intention. Thus, I cecided to refrain from guessing (and potentially breaking) their meaning. I think, [my comment from #16319](https://github.com/apache/nuttx/pull/16319#issuecomment-2870029622) applies here, as well: I think, I prepared the spelling fixes commits with a good amount of care and diligence. But I do not think, we should burden my commits for "fixing 99% of spelling issues" with the task of really fixing 100% of the issues, just because I introduce the new CI checks *before* all of my commits were merged. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2868336497 > I would like to wrap this PR up. Is there anything missing? still has error not fix yet: https://github.com/apache/nuttx/actions/runs/14902428691/job/41857195862?pr=16318 https://github.com/apache/nuttx/actions/runs/14902428711/job/41857195812?pr=16318 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2867509007 I would like to wrap this PR up. Is there anything missing? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2862312868 > please fix the conflict. done -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2862246153 please fix the conflict. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2862244574 Following @xiaoxiang781216's suggestion, I prolonged the three release note lines instead of adding exceptions. The remaining exceptions for release note lines refer to commit titles *deliberately* mentioning the misspelling of a word. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2854683859 If we already modify Release note, I prefer to fix all problems directly instead adding the exception rule. It's always good to minimize the exception rule as much as possible. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318:
URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2854536217
I am trying to summarize the state of discussion:
* A) Enabling codespell checks in `checkpatch.sh` by default?
* At the moment, *no tests* are enabled by default in that script. Maybe
we want to change this in a separate PR.
* This PR does not do anything in this field.
* See #16302 for codespell/CI-related recent changes.
* B) Should spelling mistakes in release notes be fixed?
* The release notes are automatically created based on commit titles.
* See the [(short) discussion in another
PR](https://github.com/apache/nuttx/pull/16282#discussion_r2065314637).
* This PR fixes spelling issues in release notes (i.e. release notes are
treated as "mutable").
* C) Prolonging truncated lines in release notes in case of spelling issues?
* Some commit titles are too long. These are truncated, which may cause
the last word to be detected by `codespell` as a typo.
* We could either fix spelling (i.e. raise the character limit per line)
or we add this truncated line to the list of codespell exceptions
(`.codespell-ignore-lines`).
* In this PR I picked the latter approach ("exception instead of
prolonging").
Personally I do not have a strong opinion for any of the points above. Thus,
if there is a desire to change any of the points above, I will happily adjust
the PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2854250308 > I think that the release notes should not be corrected since those are spelling errors in the actual commit titles but the similar issue in commit message is already fixed in previous patch. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2852038376 hint: updated PR with one more spelling fix (`MingGW`) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
acassis commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2852015092 Thank you @sumpfralle now I saw the point, the -c parameter already existed in checkpatch.sh. The issue is: in the past it was optional, since there is no codespell check in the CI. Since now it is mandatory my point it checkpatch.sh should run code spell by default without requiring user to pass -c -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2852005777 > @sumpfralle I looked again and didn't see it included inside checkpatch.sh script. > > More info about the script: https://nuttx.apache.org/docs/latest/components/tools/index.html#checkpatch-sh > > Basically before the user open a PR they should run checkpatch.sh to find issues, so normally users don't need to run the CI locally, but just checkpatch.sh to spot issues I have the feeling, we are confusing each other :) My understanding is the following: * `checkpatch.sh` can be called with the parameter `-c` in order to run codespell * the pre-commit hook calls `checkpatch.sh` with the `-c` parameter (since #16302) * the CI workflow `check` calls `checkpatch.sh` with the `-c` parameter I am not sure, what I am missing here. Maybe you want to enable the codespell check in `checkpatch.sh` by default? (i.e. without requiring `-c`) -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
acassis commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2851981753 > > I think now the checkpatch should include the code spelling test, [..] > > I think, this was done in #16302? Or do you mean something else? @sumpfralle I looked again and didn't see it included inside checkpatch.sh script. More info about the script: https://nuttx.apache.org/docs/latest/components/tools/index.html#checkpatch-sh Basically before the user open a PR they should run checkpatch.sh to find issues, so normally users don't need to run the CI locally, but just checkpatch.sh to spot issues -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2851955035 > I think now the checkpatch should include the code spelling test, [..] I think, this was done in #16302? Or do you mean something else? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
acassis commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2851946384 @sumpfralle @xiaoxiang781216 I think now the checkpatch should include the code spelling test, otherwise devs only will discover about the fault too late (then it is using the CI) and we will waste CI processing time to find issues that users should have catch early -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
sumpfralle commented on code in PR #16318: URL: https://github.com/apache/nuttx/pull/16318#discussion_r2073738401 ## .codespell-ignore-lines: ## @@ -12,3 +12,10 @@ Linix 45ZWN24-40 2 0.5 Ohm0.400 mH 2.34A 24V THN, # define AES_ISR_URAT_WORRDACC (5 << AES_ISR_URAT_SHIFT) /* WRONLY register read access */ WRONLY, +* [#4739](https://github.com/apache/nuttx/pull/4739) Revert "lib/netdb: Change the default NETDB_DNSCLIENT_NAMESIZE to NAM… Review Comment: The lines in the release notes were arbitrarily truncated to a certain length. If we want to allow longer release note lines, then we should change the script, which produces them. I am undecided here. If you think, longer release note lines are acceptable, then I can fix the corresponding lines instead of adding exceptions. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
nuttxpr commented on PR #16318: URL: https://github.com/apache/nuttx/pull/16318#issuecomment-2851468744 [**\[Experimental Bot, please feedback here\]**](https://github.com/search?q=repo%3Aapache%2Fnuttx+13552&type=issues) __Fill In The Commit Message:__ This PR contains a Commit with an Empty Commit Message. Please fill in the Commit Message with the PR Summary. No. The PR description lacks crucial information required by the NuttX guidelines. Specifically: * **Summary:** While it mentions the changes, it lacks a clear explanation of *how* the changes work (e.g., "Corrected misspelled words X, Y, and Z in files A, B, and C using codespell."). Referencing other PRs is helpful context, but insufficient. * **Impact:** While mentioning reduced typos is good, it's too general. The template asks for specific impacts on users, build, hardware, documentation, security, and compatibility. Even if the answer is "NO" for most, it needs to be explicitly stated. For documentation, a "YES" is appropriate here, with a description like "Improved clarity and correctness of the documentation." * **Testing:** "Tested on myself" is not sufficient. The template requires details about the build host and target(s) used for testing, as well as testing logs *before* and *after* the changes. While readability is subjective, providing examples of corrected typos in the logs would significantly strengthen the testing section. Even if the change is purely documentation, build verification on relevant platforms should be included. In short, the PR needs to be more thorough and explicitly address each point in the template, even if the answer is a simple "NO." -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
Re: [PR] Documentation: fix spelling [nuttx]
xiaoxiang781216 commented on code in PR #16318: URL: https://github.com/apache/nuttx/pull/16318#discussion_r2073708462 ## .codespell-ignore-lines: ## @@ -12,3 +12,10 @@ Linix 45ZWN24-40 2 0.5 Ohm0.400 mH 2.34A 24V THN, # define AES_ISR_URAT_WORRDACC (5 << AES_ISR_URAT_SHIFT) /* WRONLY register read access */ WRONLY, +* [#4739](https://github.com/apache/nuttx/pull/4739) Revert "lib/netdb: Change the default NETDB_DNSCLIENT_NAMESIZE to NAM… Review Comment: why not fix the spelling directly? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
