Check default doctest directives in CI #17269

lucyleeow · 2020-05-18T15:50:01Z

adrinjalali · 2020-05-18T18:36:58Z

lucyleeow · 2020-05-19T10:58:47Z

jnothman · 2020-05-19T11:04:43Z

jnothman

jnothman · 2020-05-19T11:06:04Z

build_tools/circle/linting.sh

+
+if [ ! -z "$doctest_directive" ]
+then
+    echo "Default doctest directives found:"


Let's make it clearer what the issue is:

Suggested change

echo "Default doctest directives found:"

echo "ELLIPSIS and NORMALIZE_WHITESPACE doctest directives are enabled by default, but were found in:"

Thanks, I wasn't sure how much information to give.

I expect the message will be rarely seen, which is only more reason to provide detail for when it surprises someone!

adrinjalali · 2020-05-19T12:34:30Z

build_tools/circle/linting.sh

+
+# Check for default doctest directives ELLIPSIS and NORMALIZE_WHITESPACE
+
+doctest_directive="$(find . -type f \( -name "*.py" -o -name "*.rst" \) -exec grep -nw -E "# doctest: \+(ELLIPSIS|NORMALIZE_WHITESPACE)" {} \;)"


wouldn't git grep -nw -E "# doctest\: \+(ELLIPSIS|NORMALIZE_WHITESPACE)" be much faster?

I didn't know about git grep. There is no way to search only in .py and .rst files but I don't think that is going to be a problem. Though it means searching through more files than required.
I didn't trust grep to do the file selection as I've had problems, which is why I used find. Plus this way find and grep are working in parallel.
Not a bash expert, I can test locally and see which is faster?

I tested and git grep seems to be much faster, even though it's going through more files. We also don't have any occurrence of # doctest +(ELLIPSIS|NORMALIZE_WHITESPACE) anywhere anyway, so grep going though more files shouldn't be an issue.

Your way is way faster!

lucyleeow added 3 commits May 18, 2020 17:48

add grep

12b71e8

directs to test CI

420baff

attempt2

43fd775

lucyleeow added 3 commits May 18, 2020 22:37

better output format

fa87a1e

use find

cdef6b4

revert test rst file

95ee9d9

lucyleeow changed the title ~~[WIP] Check default doctest directives in CI~~ Check default doctest directives in CI May 19, 2020

jnothman approved these changes May 19, 2020

View reviewed changes

better message

84add85

adrinjalali reviewed May 19, 2020

View reviewed changes

faster grep

b919261

adrinjalali approved these changes May 19, 2020

View reviewed changes

adrinjalali merged commit 2d12bb5 into scikit-learn:master May 19, 2020

lucyleeow deleted the IS/17019 branch May 20, 2020 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Check default doctest directives in CI #17269

Check default doctest directives in CI #17269

	echo "Default doctest directives found:"
	echo "ELLIPSIS and NORMALIZE_WHITESPACE doctest directives are enabled by default, but were found in:"


		# Check for default doctest directives ELLIPSIS and NORMALIZE_WHITESPACE

		doctest_directive="$(find . -type f \( -name ".py" -o -name ".rst" \) -exec grep -nw -E "# doctest: \+(ELLIPSIS\|NORMALIZE_WHITESPACE)" {} \;)"

Check default doctest directives in CI #17269

Check default doctest directives in CI #17269

Conversation

Reference Issues/PRs

What does this implement/fix? Explain your changes.

Any other comments?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment