Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix various typos in docs/ subdirectory #2550

Merged
merged 2 commits into from
Jun 30, 2023
Merged

Conversation

luzpaz
Copy link
Contributor

@luzpaz luzpaz commented Jun 28, 2023

Fixes to english docs/*.adoc documentation files
Follow-up to #2548

Fixes to english docs/*.adoc documentation files
docs/src/gcode/coordinates.adoc Outdated Show resolved Hide resolved
@SebKuzminsky
Copy link
Collaborator

Thanks for these fixes.

Not as part of this PR, but in general, maybe it would be good to have a script that checks if there are any spelling errors. We could run it in the Buildbot and the GitHub Actions and reject PRs that introduce spelling errors...

@luzpaz
Copy link
Contributor Author

luzpaz commented Jun 29, 2023

Thanks for these fixes.

You're welcome. Thank you for all you work on this project.

Not as part of this PR, but in general, maybe it would be good to have a script that checks if there are any spelling errors. We could run it in the Buildbot and the GitHub Actions and reject PRs that introduce spelling errors...

Indeed, check out https://github.com/codespell-project/actions-codespell

@c-morley
Copy link
Collaborator

I disagree with automatic blocking of prs because of spelling mistakes.
An automatic report would be fine though.

@SebKuzminsky
Copy link
Collaborator

I disagree with automatic blocking of prs because of spelling mistakes.
An automatic report would be fine though.

How do you propose these reports would be generated and communicated?

We currently have implemented a policy (aligned with your opinion and aligned against mine) that failures in our automatic build tests do not prevent us from merging a PR.

Does this mean you're ok with adding a spelling check to our GitHub Actions and to the Buildbot? The result of the spelling check would show up along with the results of all the existing tests we have; in the red X indicating failure or the green checkmark indicating success. But just like now, a developer could choose to ignore the "red X" failure and merge the problematic PR anyway.

@hansu
Copy link
Member

hansu commented Jun 29, 2023

I am ok with adding a spelling check to our GitHub Actions. Yes we can ignore a failure on that action if a PR need to be merged quickly, but on the long run it would be better to add an exception for that reported spelling error (or just correct it if possible).

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Jun 29, 2023 via email

@luzpaz
Copy link
Contributor Author

luzpaz commented Jun 29, 2023

Just a note regarding the codespell as a github action:
It does give some false positives, so spellcheck failure auto-blocked PRs are not ideal because of this.

@c-morley
Copy link
Collaborator

I disagree with automatic blocking of prs because of spelling mistakes.
An automatic report would be fine though.

How do you propose these reports would be generated and communicated?

We currently have implemented a policy (aligned with your opinion and aligned against mine) that failures in our automatic build tests do not prevent us from merging a PR.

Does this mean you're ok with adding a spelling check to our GitHub Actions and to the Buildbot? The result of the spelling check would show up along with the results of all the existing tests we have; in the red X indicating failure or the green checkmark indicating success. But just like now, a developer could choose to ignore the "red X" failure and merge the problematic PR anyway.

I am saying I see the utility of spell checking. I don't know the best way to offer that information to the developer.
But I am also saying that having slightly misspelled docs is better then no docs.

1 similar comment
@c-morley
Copy link
Collaborator

I disagree with automatic blocking of prs because of spelling mistakes.
An automatic report would be fine though.

How do you propose these reports would be generated and communicated?

We currently have implemented a policy (aligned with your opinion and aligned against mine) that failures in our automatic build tests do not prevent us from merging a PR.

Does this mean you're ok with adding a spelling check to our GitHub Actions and to the Buildbot? The result of the spelling check would show up along with the results of all the existing tests we have; in the red X indicating failure or the green checkmark indicating success. But just like now, a developer could choose to ignore the "red X" failure and merge the problematic PR anyway.

I am saying I see the utility of spell checking. I don't know the best way to offer that information to the developer.
But I am also saying that having slightly misspelled docs is better then no docs.

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Jun 29, 2023 via email

@c-morley
Copy link
Collaborator

Because if the process has too much 'friction' ie the PRs sit for too long before being approved it discourages people investing time into linuxcnc.

Spelling mistakes while annoying are not going to break limuxcnc'

@c-morley c-morley closed this Jun 29, 2023
@c-morley c-morley reopened this Jun 29, 2023
@c-morley
Copy link
Collaborator

Sorry -butter fingers on phone..

...not going to break linuxcnc's use.
If the developer doesn't fix the spelling mistakes then we don't get the docs (so no docs) or the developer doesn't even bother with the PR because the process is painful ( so no docs) or the docs are held up for months(?) Till someone fixes the dozen spelling mistakes (so no docs for a long time)

Particularly talking of new or one time developer PRs.

@petterreinholdtsen
Copy link
Collaborator

petterreinholdtsen commented Jun 30, 2023 via email

@hansu
Copy link
Member

hansu commented Jun 30, 2023

I think we should give it a try and add it as a GitHub action.
If one doesn't want to deal with spelling erros, he can ignore that failed check and merge it anyways. The downside is only that from this merge all builds will fail at first sight (if not looked at the specific steps) until the spelling errors are fixed.

@c-morley
Copy link
Collaborator

So if I understand correctly, the underlying problem is that the feedback period is long, and almost no-one respond to pull requests to the author in a timely manner. I would guess the solution would be to focus more on providing feedback to pull requests quicker, not reduce the quality control.

-- Happy hacking Petter Reinholdtsen

Sure that would help a lot- how do you propose we do that?
We already tried forcing reviews of every PR and that didn't seem to work out very well.

Having a policy of only including 'perfect code' is a great idea in theory.
But in practice we just don;t have enough people in the right places to so that.

A policy of trying to stop code that breaks linuxcnc builds/functions but allows some minor stuff like miss spelling is just a practical fact of running a small nobody-gets-paid volunteer project.

Hopefully this check could be set up to announce spelling mistakes as warnings not errors as not to dilute the required attention error messages mean.

@andypugh andypugh merged commit 47da39c into LinuxCNC:2.9 Jun 30, 2023
@luzpaz luzpaz deleted the typos-docs branch June 30, 2023 19:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants