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

Add codespell support (config, workflow to detect/not fix) and make it fix few typos #23398

Merged
merged 3 commits into from
Sep 5, 2024

Conversation

yarikoptic
Copy link
Contributor

codespell is not new to this project but not really part of the automated workflow(s) (precommit, CI).

There is Makefile rule but had hardcoded settings into it, so people had to use make and could not pre-commit or directly -- I moved configuration into pyproject.yaml so now if you have codespell (and tomli) installed - you can just codespell.

CI workflow has 'permissions' set only to 'read' so also should be safe.

Also added pre-commit hook for it so no new typos would sneak in.

attn @rhatdan as I believe the codespell user ;)

Copy link
Contributor

openshift-ci bot commented Jul 24, 2024

Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

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-sigs/prow repository.

@openshift-ci openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Jul 24, 2024
@rhatdan rhatdan added No New Tests Allow PR to proceed without adding regression tests and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Jul 25, 2024
Copy link
Member

@Luap99 Luap99 left a comment

Choose a reason for hiding this comment

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

Thanks, I am fine with having codespell in CI but you properly want to wait for feedback from other maintainers first before spending more time on this in case others do not like it

Copy link
Member

Choose a reason for hiding this comment

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

We do not use github actions for the most part so please delete that.

If we want to enforce this on CI it must run in cirrus as part of make validate-sources, given you already hooked this into pre-commit this is already the case so it works

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will

  • delete GH action
  • add a TEMP commit with a typo to ensure they do get detected with pre-commit (will take PR into draft for the duration of TEMP commit presence).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apparently both "Cirrus CI / Windows Cross" and "Cirrus CI / Validate source code changes" ran pre-commit and failed on codespell. As the point of test catching new typos is fulfilled, I will remove TEMP commit now and take out of draft.

pyproject.toml Outdated
@@ -13,3 +13,10 @@ exclude = '''
| hack
)/
'''

[tool.codespell]
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that, it feels totally arbitrary to put this unrelated configuration in a python config file.

I recommend to use .codespellrc

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Up to you. I am even a little on your side since then it would not require optional tomli.

Just 1 more chance to reconsider under the following arguments

So let me know

Copy link
Member

Choose a reason for hiding this comment

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

I don't want this in a random python file, we are not a python project so please remove it from there. I don't think anyone will ever look into this file ever and fine the codespell config there.

@rhatdan
Copy link
Member

rhatdan commented Jul 25, 2024

I like this alot, thanks @yarikoptic. Fix the couple of issues @Luap99 found, and I will give it a LGTM

@Luap99
Copy link
Member

Luap99 commented Jul 25, 2024

Also if you repush can you please fix up the commit messages for the [DATALAD RUNCMD] commits, that is completely useless data for any human reader of the commit messages

@rhatdan
Copy link
Member

rhatdan commented Jul 25, 2024

I would be fine with just merging the commits into one PR.

@yarikoptic
Copy link
Contributor Author

yarikoptic commented Jul 25, 2024

Also if you repush can you please fix up the commit messages for the [DATALAD RUNCMD] commits, that is completely useless data for any human reader of the commit messages

FWIW: I can easily adjust short commit message and can remove internal record on how the modification was made (some humans are curious creatures and actually do like to see command line invocation which resulted in the committed diff... More on datalad run at https://handbook.datalad.org/en/latest/basics/basics-run.html ... It also becomes "fun" to do "functional rebase" whenever commit is taken not as a diff, but rather as "do this action" (like codespell -w or some sed -i invocation), thus avoiding dealing with conflicts or simply missing some new changes to be done.

With that in mind, just tell me final verdict of

  • C1: leave as is
  • C2: adjust short summary
  • C3: adjust full commit message and remove embedded json record with the command

@yarikoptic yarikoptic marked this pull request as draft July 25, 2024 21:12
@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2024
@yarikoptic yarikoptic marked this pull request as ready for review July 25, 2024 21:25
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 25, 2024
@yarikoptic yarikoptic requested a review from Luap99 July 25, 2024 21:25
@rhatdan
Copy link
Member

rhatdan commented Aug 2, 2024

LGTM
@Luap99 PTAL

@Luap99
Copy link
Member

Luap99 commented Aug 2, 2024

C3: adjust full commit message and remove embedded json record with the command

I am reading commit message and this random stuff just wastes every readers time trying to make sense of this and why it is there. Please just squash the codespell fixes in a commit and name it something like fix issues reported by codespell or something like this.

@@ -309,7 +309,7 @@ test/version/version: version/version.go

.PHONY: codespell
codespell:
codespell -S bin,vendor,.git,go.sum,.cirrus.yml,"*.fish,RELEASE_NOTES.md,*.xz,*.gz,*.ps1,*.tar,swagger.yaml,*.tgz,bin2img,*ico,*.png,*.1,*.5,copyimg,*.orig,apidoc.go" -L ddress,secon,passt,bu,hastable,te,clos,ans,pullrequest,uint,iff,od,seeked,splitted,marge,erro,hist,ether,specif -w
codespell -w
Copy link
Member

Choose a reason for hiding this comment

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

it would help to add a comment here saying where the new config is for authors used to edit the rules here inline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added a comment

@yarikoptic
Copy link
Contributor Author

  • moved config into new .codespellrc
  • rebased and reran fixing (there was one new typo)
  • squashed multiple commits and minimized their commit messages - now only 3 commits. Feel welcome to squash into a single one upon merging

Copy link

github-actions bot commented Sep 5, 2024

A friendly reminder that this PR had no activity for 30 days.

@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2024

@yarikoptic still working on this? Looks like all it needs is to adjust the commit message.

@yarikoptic
Copy link
Contributor Author

Well, I thought I was done with it! ;) now spotted that failed CI apparently complaining about overly long subject line!

 * 9bde6b551 "Add codespell config, pre-commit definition, and move used options from Makefile into config" ... FAIL
  - FAIL - commit subject exceeds 90 characters

ok,

  • reworded
  • Since that rewrote history -- rebased again
  • saw that 2 new typos detected, made it fix those and fixup that last commit with other fixes
  • force pushed

@rhatdan rhatdan removed the stale-pr label Sep 5, 2024
@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2024

/approve
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2024
Copy link
Contributor

openshift-ci bot commented Sep 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan, yarikoptic

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-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2024
@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2024

Thanks @yarikoptic

@openshift-merge-bot openshift-merge-bot bot merged commit ae14dff into containers:main Sep 5, 2024
87 of 88 checks passed
@stale-locking-app stale-locking-app bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Dec 5, 2024
@stale-locking-app stale-locking-app bot locked as resolved and limited conversation to collaborators Dec 5, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. No New Tests Allow PR to proceed without adding regression tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants