-
Notifications
You must be signed in to change notification settings - Fork 93
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
add typos spell checker to pre-commit #2568
Conversation
@@ -606,7 +606,7 @@ class NodeSelectorKeyValue(schema.Base): | |||
|
|||
class KubernetesCredentials(schema.Base): | |||
host: str | |||
cluster_ca_certifiate: str | |||
cluster_ca_certificate: str |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should catch this in the update step and update the spelling for users if they had this set explicitly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would add a # typos: ignore
here for now, and a todo to address this for next release, so that we add as part of the update method as suggested. to keep this in mind, we could pin the issue in our notes, or in the issues tab
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We will likely need to delete the old file during the nebari update step since otherwise it'll leave it behind in the stages folder. We've done this in the past.
Even though this isn't done yet (see my comments), I'd be curious to get any feedback on it @dcmcand @marcelovilla @viniciusdc |
@Adam-D-Lewis thank you for this! I wouldn't have thought we had that many typos in the code base and it's great you're adding a hook to catch them. I'm not familiar with either As for fixing the typo in the configuration file, I agree we should include the logic to fix the spelling in the user's config file and delete the rendered terraform file during the upgrade step. However, I don't think it's necessarily urgent and we could have a separate PR for that if you think it's more convenient. |
I completely agree on what @marcelovilla said above as well, and would vote for a separate PR to address the changes in the config file. |
Originally found detected/fixed by typos in - nebari-dev/nebari#2568
Originally found detected/fixed by typos in - nebari-dev/nebari#2568
@viniciusdc Alright, I pushed off the 2 things which would take a bit more work to resolve and opened issues for them so we can get this in sooner rather than later. @viniciusdc or @marcelovilla Can you review again? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, nice tool! 🚀
Reference Issues or PRs
Adds typos to the pre-commit config to fix a lot of uncaught typos.
I think the biggest benefit of adding typos is that it caught quite a few typos that codespell doesn't catch. I'm okay with trying to leave codespell and typos both in the pre-commit file, but they may conflict eventually. If that's the case, perhaps we remove either codespell or typos at that time from the pre-commit file.
Typos is faster than codespell. Typos took 0.2 seconds to run while typos took 0.05 seconds to run in my test, but I think the time difference is not important since they are both quick to run.
What does this implement/fix?
Put a
x
in the boxes that applyTesting
Any other comments?