Skip to content
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

Merged
merged 4 commits into from
Mar 13, 2023
Merged

Conversation

nalimilan
Copy link
Member

We cannot just use strict=true currently as some docstrings are considered as missing.

We cannot just use `strict=true` currently as some docstrings
are considered as missing.
Copy link
Member

@kleinschmidt kleinschmidt left a 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...

@nalimilan
Copy link
Member Author

Woops. Should be fixed now.

Copy link
Member

@kleinschmidt kleinschmidt left a 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...

@nalimilan
Copy link
Member Author

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.

@nalimilan
Copy link
Member Author

Note that doctest failures are already annoying in PRs as they are shown in the files view, even for untouched files.

@kleinschmidt
Copy link
Member

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...)

@kleinschmidt
Copy link
Member

merged master and will re-run CI

@kleinschmidt kleinschmidt self-requested a review March 13, 2023 22:31
@kleinschmidt kleinschmidt merged commit b671d12 into master Mar 13, 2023
@kleinschmidt kleinschmidt deleted the nl/doctests branch March 13, 2023 22:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants