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 typos #326

Conversation

gibizer
Copy link
Contributor

@gibizer gibizer commented Apr 3, 2023

This PR is motivated by the fix openstack-k8s-operators/lib-common#231

Depends-on #280 (merged)

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 3, 2023

Skipping CI for Draft Pull Request.
If you want CI signal for your change, please convert it to an actual PR.
You can still manually trigger a test run with /test all

@openshift-ci openshift-ci bot added the approved label Apr 3, 2023
This PR is motivated by the fix openstack-k8s-operators/lib-common#231
@gibizer gibizer force-pushed the pr-280-operator-bump branch from d080325 to 47e441e Compare April 5, 2023 08:47
@gibizer gibizer marked this pull request as ready for review April 5, 2023 08:48
@openshift-ci openshift-ci bot requested review from bogdando and mrkisaolamb April 5, 2023 08:48
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 5, 2023

@gibizer: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/nova-operator-build-deploy 47e441e link false /test nova-operator-build-deploy

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

Copy link
Contributor

@mrkisaolamb mrkisaolamb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! Have you considered adding some tooling for this kind of check, like the 'misspell' linter from golangci-lint? It can check all committed files and report the most common misspellings. It also has an ignore option if we want to avoid correction of some words. I ran it locally and got quite similar results.

@SeanMooney
Copy link
Contributor

im warey of addign tooling for spelling as it will disproportionately affect me.
i will often have spelling issues in comment ectra and i don't want that to block me committing a change locally as somthime that misspelling will be nova or pre-commit or some other term like vdpa. i don't really want to have to maintain a dictionary of those in three or otherwise have to work around the tooling.

we could try it but to me typos in code comments are not a reason to reject a patch.

if it does not affect the functioning of the code it is not a bug or code quality issue.
that is what i want to use linter for.

we could try it but if its makes it significantly harder for me to work on this repo i would like to be able to remove it in the future.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Apr 5, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: gibizer, SeanMooney

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 701b992 into openstack-k8s-operators:master Apr 5, 2023
@mrkisaolamb
Copy link
Contributor

im warey of addign tooling for spelling as it will disproportionately affect me. i will often have spelling issues in comment ectra and i don't want that to block me committing a change locally as somthime that misspelling will be nova or pre-commit or some other term like vdpa. i don't really want to have to maintain a dictionary of those in three or otherwise have to work around the tooling.

we could try it but to me typos in code comments are not a reason to reject a patch.

if it does not affect the functioning of the code it is not a bug or code quality issue. that is what i want to use linter for.

we could try it but if its makes it significantly harder for me to work on this repo i would like to be able to remove it in the future.

Sure, I understand what you mean. I have the same issue (it's even more noticeable in Polish). I saw that the tool has an autocommit option, so in theory it can correct errors automatically, but I haven't tested it yet. If this check is annoying, I'll drop the question.

@gibizer
Copy link
Contributor Author

gibizer commented Apr 5, 2023

I have a spell check plugin in vscode locally but my reason of this change wasn't to fix typos in code / code comments, but to fix typos in external facing messages, like our condition status messages. That could be seen by the end user and having typos there is a marketing thing, I guess. :) But then I get carried away and fixed the code typos as well for consistency. My experience with automatic spell checking is that it needs a project / codebase specific allowlist to not being a blocker, and keeping that allow list sane is annoying.

Bottom line: If having an automated spellcheck with a manually maintained allowlist is not a burden then lets go for it, but I afraid it will be.

@gibizer gibizer deleted the pr-280-operator-bump branch May 31, 2023 13:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants