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

ci: add codespell check script for tracking typos #12232

Merged
merged 2 commits into from Oct 10, 2019

Conversation

aabadie
Copy link
Contributor

@aabadie aabadie commented Sep 14, 2019

Contribution description

This PR introduce a new static check script for tracking typos in PRs.

Testing procedure

Issues/PRs references

Should help with #12230

@aabadie aabadie added Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools labels Sep 14, 2019
@aabadie aabadie force-pushed the pr/tools/codespell_check branch 2 times, most recently from 1036ce6 to 28a36ad Compare September 17, 2019 12:25
@aabadie aabadie force-pushed the pr/tools/codespell_check branch 4 times, most recently from 0335f3c to 700a009 Compare September 24, 2019 09:34
@kaspar030
Copy link
Contributor

There's a lot of force-pushing going on here. ;)

@aabadie
Copy link
Contributor Author

aabadie commented Sep 24, 2019

There's a lot of force-pushing going on here. ;)

Yeah, I wanted to verify if the container was running the right version. Now everything seems ok. When the Murdock workers are updated, this will be ready for merge (after reviews of course).

@aabadie aabadie added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Oct 2, 2019
@kaspar030
Copy link
Contributor

Ah, this was the other PR waiting on CI update ;)

@aabadie
Copy link
Contributor Author

aabadie commented Oct 10, 2019

Now it works also on Murdock!

@kaspar030
Copy link
Contributor

I tried this on master, there are ~1000 hits, most of them typos but some false positives ("od" -> "of", "WAN" -> "WANT").

Can we change the return value in case of errors to "0", so it basically becomes a warning? With that I'd be OK to merge.

Then I think we need an "--exclude-file" so we can filter the false positives, fix all actual typos and then make this use live ammunition (exit 1 on typo). (but in follow-up PRs).

@aabadie
Copy link
Contributor Author

aabadie commented Oct 10, 2019

most of them typos but some false positives ("od" -> "of", "WAN" -> "WANT")

We can add them to a list of excluded patterns (see here).

Can we change the return value in case of errors to "0", so it basically becomes a warning?

OK

I tried this on master, there are ~1000 hits

If you call the script directly, all files are parsed but using make static-test only files changed in the current branch compared to master are parsed.

@kaspar030
Copy link
Contributor

We can add them to a list of excluded patterns (see here).

Well, we'd need a file for that, too, then (list will get long fast). But wouldn't doing that reduce effectiveness? While some cases might want to use "od" instead of "of", that might also be a real typo. Using "--exclude-file", we have to explicitly add every false-positive line, but codespell stays sharp.

@kaspar030
Copy link
Contributor

If you call the script directly, all files are parsed but using make static-test only files changed in the current branch compared to master are parsed.

Yes, and it shows typos that were already in a file, when doing a probably unrelated change...

@kaspar030
Copy link
Contributor

OK

You can squash directly!

@aabadie
Copy link
Contributor Author

aabadie commented Oct 10, 2019

squashed

Copy link
Contributor

@kaspar030 kaspar030 left a comment

Choose a reason for hiding this comment

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

ACK.

@kaspar030 kaspar030 merged commit bfebb73 into RIOT-OS:master Oct 10, 2019
@aabadie aabadie deleted the pr/tools/codespell_check branch October 10, 2019 15:41
@aabadie
Copy link
Contributor Author

aabadie commented Oct 10, 2019

Thank you!

@kb2ma kb2ma added this to the Release 2019.10 milestone Oct 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants