-
Notifications
You must be signed in to change notification settings - Fork 32
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
Update doctests and make CI fail if they don't pass #272
Conversation
We cannot just use `strict=true` currently as some docstrings are considered as missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
doctest updates look good, but I'm not seeing the "make CI fail" part...
Woops. Should be fixed now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I sorta thought that we didn't want to fail our builds for failing doctests, since they're not just testing stuff in our package but printing in base/other packages which can change without a breaking version bump.
At some point I think we'd discussed using something liek reviewdog + doctest(; fix=true)
to surface changes during PR review. That maybe would catch some churn from the filtered things that we know will change (dict ordering etc.) but would make it possible to keep the doctests up-to-date without suprious build failures unrelated to PR changes...
Yeah ideally a bot could handle this. But waiting for that I'd say it's better that we notice when doctests are out of date. Otherwise we never check them and we're not even sure they run without errors. |
Note that doctest failures are already annoying in PRs as they are shown in the files view, even for untouched files. |
We could add this action as a workflow step: https://github.com/julia-actions/julia-fix-doctests it works on PRs where there's a "fix doctests" label (and removes teh label afterwards...) |
merged master and will re-run CI |
We cannot just use
strict=true
currently as some docstrings are considered as missing.