Check default doctest directives in CI#17269
Check default doctest directives in CI#17269adrinjalali merged 8 commits intoscikit-learn:masterfrom
Conversation
|
Do we want to keep the circleci and azure linting in sync? @thomasjpfan @rth maybe? |
|
Uses |
circle/linting.sh is used in azure. |
build_tools/circle/linting.sh
Outdated
|
|
||
| if [ ! -z "$doctest_directive" ] | ||
| then | ||
| echo "Default doctest directives found:" |
There was a problem hiding this comment.
Let's make it clearer what the issue is:
| echo "Default doctest directives found:" | |
| echo "ELLIPSIS and NORMALIZE_WHITESPACE doctest directives are enabled by default, but were found in:" |
There was a problem hiding this comment.
Thanks, I wasn't sure how much information to give.
There was a problem hiding this comment.
I expect the message will be rarely seen, which is only more reason to provide detail for when it surprises someone!
build_tools/circle/linting.sh
Outdated
|
|
||
| # 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)" {} \;)" |
There was a problem hiding this comment.
wouldn't git grep -nw -E "# doctest\: \+(ELLIPSIS|NORMALIZE_WHITESPACE)" be much faster?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Your way is way faster!
* add grep * directs to test CI * attempt2 * better output format * use find * revert test rst file * better message * faster grep
* add grep * directs to test CI * attempt2 * better output format * use find * revert test rst file * better message * faster grep
Reference Issues/PRs
closes #17019
What does this implement/fix? Explain your changes.
Any other comments?