Skip to content

dist/tools/doccheck: add check for undefined groups#9266

Merged
miri64 merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/tool/doc_group_check
Jun 7, 2018
Merged

dist/tools/doccheck: add check for undefined groups#9266
miri64 merged 1 commit intoRIOT-OS:masterfrom
aabadie:pr/tool/doc_group_check

Conversation

@aabadie
Copy link
Copy Markdown
Contributor

@aabadie aabadie commented Jun 1, 2018

Contribution description

As requested in #9264, this PR update the doxygen check script with a check on undefined groups.

For the moment, it's run only if doxygen documentation passes without error and it always returns 0. The undefined groups are printed with a warning message at the beginning.

Issues/PRs references

Related to #9264 and #8972

@aabadie aabadie added Area: doc Area: Documentation CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Area: CI Area: Continuous Integration of RIOT components Area: tools Area: Supplementary tools labels Jun 1, 2018
@aabadie aabadie requested a review from miri64 June 1, 2018 19:34
@aabadie aabadie force-pushed the pr/tool/doc_group_check branch from 589aba2 to 7f7ac60 Compare June 5, 2018 12:35
UNDEFINED_GROUPS=$( \
for group in $(git grep '@ingroup' | grep -v vendor | grep -oE '[^ ]+$' | sort -u); \
do \
echo "$DEFINED_GROUPS" | grep -xq "$group" || echo "$group"; \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Might it be possible to also include the filename here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I can look at that. Not sure how complex it is.

@aabadie aabadie force-pushed the pr/tool/doc_group_check branch from 7f7ac60 to b8cbfa2 Compare June 5, 2018 14:25
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 6, 2018

Might it be possible to also include the filename here?

After quite some fight with regexp and bash, I came up with something that works locally.

DEFINED_GROUPS=$(git grep @defgroup -- '*.h' '*.txt' | \
grep -v vendor | \
grep -v examples | \
grep -v tests | \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Instead of all these line repetitions do:

exclude_filter() {
    grep -v -e vendor -e examples -e tests
}

And then use it like this in this instance

DEFINED_GROUPS=$(git grep @defgroup -- '*.h' '*.txt' | \
                  exclude_filter | \
                  grep -oE '@defgroup[ ]+[^ ]+' | \
                  grep -oE '[^ ]+$' | sort -u)

also below of course


# Check all groups are defined
DEFINED_GROUPS=$(git grep @defgroup -- '*.h' '*.txt' | \
grep -v vendor | \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Indentation could go with the git above (but I guess we have no rule for that..)

grep -v tests | \
grep -oE '[^ ]+$' | sort -u); \
do \
echo "$DEFINED_GROUPS" | grep -xq "$group" || echo "$group"; \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please use ${var} for all variables. Makes it easier to read (and AFAIK also more portable).

for group in $UNDEFINED_GROUPS; \
do \
echo -e "\n${CWARN}$group${CRESET} found in:"; \
echo "$ALL_RAW_INGROUP" | grep " $group$" | sort -u | \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think we are on the saver side if you use \<${group}\> instead of ${group}$

do \
echo -e "\n${CWARN}$group${CRESET} found in:"; \
echo "$ALL_RAW_INGROUP" | grep " $group$" | sort -u | \
awk '{ print "\t" substr($1, 1, length($1)-1) }'; \
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think awk -F':' '{ print "\t" $1 }' is way easier to understand here ;-).

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 6, 2018

@miri64, requested changes applied. Thanks!

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 7, 2018

@miri64, ok to squash here ?

@aabadie aabadie force-pushed the pr/tool/doc_group_check branch from 11f145c to a6dc4b7 Compare June 7, 2018 15:40
Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Yes, let me just give it a final test run before I ACK.

Copy link
Copy Markdown
Member

@miri64 miri64 left a comment

Choose a reason for hiding this comment

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

Works, ACK.

@aabadie aabadie force-pushed the pr/tool/doc_group_check branch from a6dc4b7 to 2f21e16 Compare June 7, 2018 17:29
@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 7, 2018

squashed

@aabadie
Copy link
Copy Markdown
Contributor Author

aabadie commented Jun 7, 2018

@miri64, CI is green. I let you press the merge button.

@miri64 miri64 merged commit fd1a987 into RIOT-OS:master Jun 7, 2018
@miri64
Copy link
Copy Markdown
Member

miri64 commented Jun 7, 2018

Done

@aabadie aabadie deleted the pr/tool/doc_group_check branch June 8, 2018 20:38
@cladmi cladmi added this to the Release 2018.07 milestone Jul 10, 2018
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: doc Area: Documentation 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.

3 participants