-
Notifications
You must be signed in to change notification settings - Fork 431
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 makefile targets to install codespell and verify using codespell #3630
Conversation
Codecov ReportPatch coverage has no change and project coverage change:
Additional details and impacted files@@ Coverage Diff @@
## main #3630 +/- ##
==========================================
- Coverage 54.05% 53.98% -0.08%
==========================================
Files 186 185 -1
Lines 18833 18700 -133
==========================================
- Hits 10181 10096 -85
+ Misses 8105 8061 -44
+ Partials 547 543 -4 ☔ View full report in Codecov by Sentry. |
This comment was marked as outdated.
This comment was marked as outdated.
6629184
to
2340280
Compare
mv $(TOOLS_BIN_DIR)/$(CODESPELL_DIST_DIR)/bin/$(CODESPELL_BIN) $(TOOLS_BIN_DIR)/$(CODESPELL_DIST_DIR); \ | ||
rm -r $(TOOLS_BIN_DIR)/$(CODESPELL_DIST_DIR)/bin; \ |
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 had to move codespell
from $(TOOLS_BIN_DIR)/$(CODESPELL_DIST_DIR)/bin/$(CODESPELL_BIN)
to $(TOOLS_BIN_DIR)/$(CODESPELL_DIST_DIR)
to avoid running into ModuleNotFoundError: No module named 'codespell_lib'
errors.
/cc @mboersma |
/retest |
/test pull-cluster-api-provider-azure-e2e-aks |
I think this makes sense, because it's not a good experience to have your PR blocked by a linter that you can't run locally first. It looks good to me overall. I'll try it out. |
If we ship this change we'll want to ship it simultaneously with a removal of the github action, otherwise we're running it twice on each PR (see the I assume the reason we've been doing the github action up until now is because no one wanted to do the chop wood / carry water work (until @nawazkh now, I mean!) of integrating this into our local CI interfaces? |
I think that's a fair ask, thanks for pointing me to it. |
2340280
to
8b5d353
Compare
8b5d353
to
4255030
Compare
Commit 8b5d353 removes the codespell GitHub action. |
4255030
to
21def33
Compare
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.
Tried this locally and it worked great, thanks @nawazkh! One small comment about adding pip to developer docs
21def33
to
6681a94
Compare
6681a94
to
a6cc9f1
Compare
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
/assign @mboersma
I really like that we're no longer using codespell @ head with this change so we won't be broken by codespell updates like we have in the past
LGTM label has been added. Git tree hash: 03803808eeed16afc928dec612cdf912d8c7ae16
|
@nawazkh any idea why the broken lints github action is failing? |
Looks like it is pointing to some broken links. I have seen such linter links fix themselves up, but I am not sure if I should update them this time.
|
a6cc9f1
to
f8fb641
Compare
- adds target to install codespell v2.2.5 at hack/tools/bin/codespell - adds a target to verify codebase with installed codespell bin - remove codespell github action
f8fb641
to
782f15d
Compare
Ready for review! :) |
/retest |
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
/approve
Tested locally, make verify-codespell
seems to work as advertised. Thanks @nawazkh!
LGTM label has been added. Git tree hash: 3a180a9160aea7ea00ae18910952168899530c34
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: mboersma 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 |
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
targets
make codespell
installscodespell
binary in the hack/tools/bin/codespell/ dirmake verify-codespell
runs$(CODESPELL) $(ROOT_DIR) --ignore-words=$(ROOT_DIR)/.codespellignore --skip="*.git,*_artifacts,*.sum,$(ROOT_DIR)/hack"
at the root of the codebase.Which issue(s) this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close the issue(s) when PR gets merged):make verify
should help users in correcting codespell errors before raising a PR.Special notes for your reviewer:
hack/tools/bin/codespell
directory. That is because installing codespell brings along other dependencies. I tried usingpip --no-deps install .. codespell
but could not get rid of other dependencies you see in the picture below.TODOs:
Release note: