Add doxygen spellcheck job to Travis #8970

kegilbert · 2018-12-05T01:15:25Z

Description

Likely for after the release.

The new job strips doxygen comments out of each header file recursively in the directory passed to the script and runs the text through aspell to provide a list of misspelled words/line number for each file. Misspelled words are counted and compared to the number of instances in just the doxygen comments versus the entire file. If the file contains more instances of the word than just the comments, it is considered a term that was used in the codebase (class name, variable, etc) and is counted as a valid word spelling.

Does not currently spell check comments in code blocks in a doxygen comment, although this would be very possible to add.

After speaking with @bridadan about where to place shell scripts in the tools to run with Travis, we decided on the present location. Please feel free to move it around.

Important files:

.travis.yml -> Travis
spell.sh -> new script
ignore.en.pws -> whitelist of words to ignore

If a user encounters a false positive during a new PR, they can add the word to the whitelist to not be counted as invalid (the error message prints a statement explaining this).

The other .dat files that were added come from the default English dictionary data files from aspell. I was not able to discover how to to use the --local-data-dir argument in aspell without all of the files present and I needed a slightly modified en.dat file. I may be able to replace the default en.dat file during setup instead of including the rest of the files, but this way was easier for people to run the test locally. Up to the maintainers/tools team on what they prefer (there also may be an aspell setting I missed that simplifies this). Let me know your thoughts.

NOTE This is expected to fail Travis as there are currently some mispellings in the tested modules. Will update the PR with typo fixes and whitelist additions once the job itself is reviewed.

Sample Job (can probably just look at the one run in this PR as well but ¯\(ツ)/¯)
https://travis-ci.org/kegilbert/mbed-os/jobs/463626330

Currently tests
- drivers
- platform
- rtos
- events
- features/netsocket

@theotherjimmy
@AnotherButler
@cmonr

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[ ] Functionality change
[ ] Docs update
[X] Test update
[ ] Breaking change

0xc0170

Also review travis failures (fixes needed for our doxy)

0xc0170 · 2018-12-05T08:46:18Z

tools/test/scripts/doxy-spellchecker/en_phonet.dat

+#
+#   This rule set is based on Lawrence Phillips original metaphone 
+#   algorithm with modifications made by Michael Kuhn in his
+#   C implantation, more modifications by Bj�rn Jacke when


can you fix the character in the name Bjorn ? Line 68 has also some problems

0xc0170 · 2018-12-05T08:46:43Z

tools/test/scripts/doxy-spellchecker/spell.sh

@@ -0,0 +1,121 @@
+#!/bin/bash


Add at least SPDX specifier here (license missing)

@0xc0170 If this needs a SPDX header, then does .travis.yml as well?

0xc0170 · 2018-12-05T08:48:24Z

Having this check will be good !

melwee01

Started looking at this, but will get back to it after lunch.

bridadan · 2018-12-05T15:39:55Z

Very nice! I'm not doing a review on this one because I'd like @theotherjimmy to ok the new scripts dir for tools/test/scripts/doxy-spellchecker/.

cmonr · 2018-12-05T15:41:02Z

https://github.com/ARMmbed/mbed-os/pull/8970/files/cc67a0b9a331a28ef2fef882aa1609666c634556..a9e1bda6ba1d7aed5f55439d61399c2e00e5d2d1

I suspect that our Finland friends might not like this PR.

@kegilbert Do you know of any way that this could be made a bit more friendly for their names?

tools/test/scripts/doxy-spellchecker/spell.sh

cmonr · 2018-12-05T17:09:21Z

tools/test/scripts/doxy-spellchecker/spell.sh

+                        fi
+
+                        while read ln; do
+                            echo "$ln $err"


Blanket recommendation, but will post here.

Would recommend adding {} to all variable uses.
Allows for the catching of undefined variables, and exits with a useful error.

Suggested change

echo "$ln $err"

echo "${ln} ${err}"

kegilbert · 2018-12-05T17:22:13Z

@cmonr

https://github.com/ARMmbed/mbed-os/pull/8970/files/cc67a0b9a331a28ef2fef882aa1609666c634556..a9e1bda6ba1d7aed5f55439d61399c2e00e5d2d1

I suspect that our Finland friends might not like this PR.

@kegilbert Do you know of any way that this could be made a bit more friendly for their names?

The linked commit was a change to the license for an aspell dict data file from @melwee01 I believe, it hasn't triggered on names yet in tested directories. The two options for ignoring names would be to manually add them to the exception list, and/or not spell check license headers (which it currently won't unless someone commented them in the doxy format which I believe we don't commonly do). Outside of licenses names shouldn't appear in the comments often I'd imagine.

melwee01 · 2018-12-05T17:24:29Z

The solution I applied isn't the most elegant, but it does match the way the guy spells his name in his email address. That only works for German names, though, as far as I'm aware.

kegilbert · 2018-12-05T17:47:37Z

The solution I applied isn't the most elegant, but it does match the way the guy spells his name in his email address. That only works for German names, though, as far as I'm aware.

I've seen it commonly done with umlauts, looks fine to me (was requested by @0xc0170 as well @cmonr due to Github rendering it looks like).

@0xc0170 Added the license file (@cmonr had a good point about the Travis file while we touching it here as well).

0xc0170 · 2018-12-10T17:00:02Z

@0xc0170 Added the license file (@cmonr had a good point about the Travis file while we touching it here as well).

We shall add license there as well (yml supports comments).

kegilbert · 2018-12-10T23:08:16Z

@AnotherButler @melwee01 Can either of you double check the new whitelist word exceptions/spelling fixes I added in the latest commit?

813a128

cmonr · 2018-12-14T01:21:29Z

Alright, getting this back on track.

@kegilbert @bridadan @theotherjimmy My thoughts on the placement of the script.

My first thought would be to create and put it into a new directory named .travis or .travis-ci, since I think I've seen other projects to the same (at least, I know Circle CI does this). However, I don't like this because the Mbed OS root is already busy as is.

My second preference then would be something like tools/test/travis-ci/<sub-job> or tools/test/travis-ci/sub-job.sh. A scripts directory within tools/test feels entirely too generic, but I'm more open to the idea if it becomes specific to Travis CI scripts.

bridadan · 2018-12-14T15:29:09Z

My second preference then would be something like tools/test/travis-ci/<sub-job>.

Fine with me.

kegilbert · 2019-01-03T18:10:26Z

@cmonr Still getting through the vacation catching up funk, which reviews are we waiting?

cmonr · 2019-01-03T20:34:26Z

Think we're only waiting on someone from docs to give the OK (ping @melwee01 or @AnotherButler), and myself, as I haven't done a deep scrub yet.

AnotherButler

I'm not sure whether the phonetic pages are necessary, but otherwise, I think this looks good 👍

kegilbert · 2019-01-08T17:51:08Z

@cmonr Is it normal for the pr-head Travis job to still be at Expected?

cmonr · 2019-01-11T05:48:25Z

NOTICE

This PR is on hold because an issue in the master branch was found when this was rebased.
Once the issue is fixed, this PR will need to be rebased, and will immediately enter CI.

adbridge · 2019-01-11T16:02:36Z

Does this really need to be coming into 5.11.2 ?

0xc0170 · 2019-01-11T16:06:02Z

I dont think it should hold 5.11.2 so rather postpone and we focus on remaining jobs

iriark01 · 2019-01-11T16:09:24Z

@adbridge we've been waiting for it for a while; any way to push it?

adbridge · 2019-01-11T16:20:00Z

@iriark01 Do you actually need it in the release or just getting pushed to master ?

Currently tests - drivers - platform - rtos - events - features/netsocket

kegilbert · 2019-01-11T16:22:55Z

Rebased. This should be fine in master, doesn't need to be in the actual release I'd think.

AnotherButler · 2019-01-11T16:26:45Z

Just getting pushed to master, please

adbridge · 2019-01-11T16:29:37Z

OK this can go back into CI once we get all the ones in for 5.11.2

NirSonnenschein · 2019-01-13T14:06:17Z

starting CI

mbed-ci · 2019-01-13T17:16:03Z

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 2
Build artifacts

0xc0170 · 2019-01-15T09:40:41Z

Before we integrate, is there an update to docs (we have testing page in the docs, that should describe this check) ?

iriark01 · 2019-01-15T09:42:09Z

Do you mean in the contributing guide, where we say what testing is done on PRs? That's a good idea.

0xc0170 · 2019-01-15T11:03:45Z

Yes, found it : https://github.com/ARMmbed/mbed-os-5-docs/blob/development/docs/reference/contributing/guidelines/ci.md - should be added here

iriark01 · 2019-01-15T12:02:56Z

I'll have to leave that to @kegilbert because I don't actually know what he's done.

kegilbert · 2019-01-15T18:00:36Z

@iriark01 @0xc0170 Put up: ARMmbed/mbed-os-5-docs#922

0xc0170 requested changes Dec 5, 2018

View reviewed changes

0xc0170 added the needs: work label Dec 5, 2018

0xc0170 requested review from AnotherButler, melwee01 and a team December 5, 2018 08:47

0xc0170 requested a review from a team December 5, 2018 08:48

melwee01 reviewed Dec 5, 2018

View reviewed changes

cmonr reviewed Dec 5, 2018

View reviewed changes

tools/test/scripts/doxy-spellchecker/spell.sh Outdated Show resolved Hide resolved

cmonr reviewed Dec 5, 2018

View reviewed changes

0xc0170 approved these changes Dec 10, 2018

View reviewed changes

kegilbert force-pushed the spell-checker-travisjob branch 2 times, most recently from 2022bfb to 3d6dc5d Compare December 10, 2018 23:05

kegilbert force-pushed the spell-checker-travisjob branch from 3d6dc5d to 813a128 Compare December 10, 2018 23:09

cmonr added needs: review and removed needs: work labels Jan 3, 2019

cmonr added the release-version: 5.11.2 label Jan 3, 2019

AnotherButler approved these changes Jan 3, 2019

View reviewed changes

cmonr added release-version: 5.11.3 needs: work and removed release-version: 5.11.2 needs: CI labels Jan 11, 2019

Add doxygen spellcheck jobs to Travis

70b9b75

Currently tests - drivers - platform - rtos - events - features/netsocket

kegilbert force-pushed the spell-checker-travisjob branch from 7ebc7b4 to 70b9b75 Compare January 11, 2019 16:22

adbridge added needs: CI and removed needs: work labels Jan 11, 2019

NirSonnenschein added ready for merge and removed needs: CI ready for merge labels Jan 13, 2019

0xc0170 added the ready for merge label Jan 14, 2019

kegilbert mentioned this pull request Jan 15, 2019

Add doxy-spellchecker info to Travis job details ARMmbed/mbed-os-5-docs#922

Merged

0xc0170 merged commit 32c9c3a into ARMmbed:master Jan 16, 2019

0xc0170 removed the ready for merge label Jan 16, 2019

Add doxygen spellcheck job to Travis #8970

Add doxygen spellcheck job to Travis #8970

Uh oh!

Conversation

kegilbert commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Pull request type

Uh oh!

0xc0170 left a comment

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

0xc0170 Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

cmonr Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

0xc0170 commented Dec 5, 2018

Uh oh!

melwee01 left a comment

Choose a reason for hiding this comment

Uh oh!

bridadan commented Dec 5, 2018

Uh oh!

cmonr commented Dec 5, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cmonr Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

melwee01 commented Dec 5, 2018

Uh oh!

kegilbert commented Dec 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

0xc0170 commented Dec 10, 2018

Uh oh!

kegilbert commented Dec 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cmonr commented Dec 14, 2018

Uh oh!

bridadan commented Dec 14, 2018

Uh oh!

kegilbert commented Jan 3, 2019

Uh oh!

cmonr commented Jan 3, 2019

Uh oh!

AnotherButler left a comment

Choose a reason for hiding this comment

Uh oh!

kegilbert commented Jan 8, 2019

Uh oh!

cmonr commented Jan 11, 2019

NOTICE

Uh oh!

adbridge commented Jan 11, 2019

Uh oh!

0xc0170 commented Jan 11, 2019

Uh oh!

iriark01 commented Jan 11, 2019

Uh oh!

adbridge commented Jan 11, 2019

Uh oh!

kegilbert commented Jan 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AnotherButler commented Jan 11, 2019

Uh oh!

adbridge commented Jan 11, 2019

kegilbert commented Dec 5, 2018 •

edited

Loading

cmonr Dec 5, 2018 •

edited

Loading

kegilbert commented Dec 5, 2018 •

edited

Loading

kegilbert commented Dec 5, 2018 •

edited

Loading

kegilbert commented Dec 10, 2018 •

edited

Loading

kegilbert commented Jan 11, 2019 •

edited

Loading