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

test: improve commons module doctests #1033

Merged
merged 37 commits into from
Oct 7, 2021
Merged

test: improve commons module doctests #1033

merged 37 commits into from
Oct 7, 2021

Conversation

bonjourmauko
Copy link
Member

@bonjourmauko bonjourmauko commented Aug 23, 2021

Fixes #1250

New Features

  • Introduce openfisca_core.types

Bug Fixes

  • Fix doctests of the commons module

Documentation

  • Complete the documentation of the commons module

Copy link
Contributor

@HAEKADI HAEKADI left a comment

Choose a reason for hiding this comment

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

Thank you @maukoquiroga! I ran the tests and everything seems to be in order :)

openfisca_core/commons/formulas.py Outdated Show resolved Hide resolved
openfisca_core/commons/tests/test_rates.py Outdated Show resolved Hide resolved
openfisca_core/commons/tests/test_rates.py Outdated Show resolved Hide resolved
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

Thank you @maukoquiroga, this is impressive documentation work! 😲 👏

How are these tests run in CI?

I would personally need the following to validate that PR:

  1. A review by at least one person who has extensive knowledge of the API and can double-check both the given information and that this is isofunctional in terms of tests. I'll ask for a review accordingly.
  2. As mentioned in a comment, that all style convention changes are extracted from this PR.
  3. That an explanation on how to run these tests and produce the documentation is added, at least to this PR, possibly to the doc itself, so that I can validate the changeset on my machine 🙂

openfisca_core/commons/rates.py Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
@MattiSG MattiSG changed the title Improve openfisca-core's commons module doctests Improve commons module doctests Aug 24, 2021
@MattiSG
Copy link
Member

MattiSG commented Aug 24, 2021

How can I confirm this would fix openfisca/openfisca-doc#244? 🙂

@bonjourmauko
Copy link
Member Author

Thanks for your comments @HAEKADI & @MattiSG

How can I confirm this would fix openfisca/openfisca-doc#244? 🙂

Good point, I'll work on it.

@bonjourmauko bonjourmauko marked this pull request as draft August 24, 2021 10:31
@bonjourmauko bonjourmauko mentioned this pull request Aug 26, 2021
@benjello
Copy link
Member

@maukoquiroga @MattiSG @HAEKADI : definitely ok to review the docstrings but I might be of better use after decisions are made about docsting format etc.

@bonjourmauko
Copy link
Member Author

How can I confirm this would fix openfisca/openfisca-doc#244? 🙂

I kind of was forced by the magics of test-driven documentation to fix it before openfisca/openfisca-doc#251

The idea would be to reach a point where builds fail in CI when the docs, including doctests, fail to build.

@bonjourmauko
Copy link
Member Author

@maukoquiroga @MattiSG @HAEKADI : definitely ok to review the docstrings but I might be of better use after decisions are made about docsting format etc.

What I intend as a first step is to make whatever exists today work, which is not the case.

That includes roughly two things:

  • Doctests, present both in the code documentation and in the markdown documentation. Both can be verified programatically so in dev and CI.

  • Documentation, where beyond doctests, you have a style —today we live with three different styles: Google, NumPy, and Shpinx—, and then prose.

    • Changes in Fix documentation openfisca-doc#251 are to be able to build the three styles (minimal possible change)
    • Changes here are to enforce Google style for new documentation
    • I don't intend to add a check for prose.

Of course, I agree with @MattiSG the aim is to automatise as much as we can, which is possible.

@bonjourmauko
Copy link
Member Author

That was some work! So I've:

  • Removed all style changes to code
  • Added pytest to automatically check that commons doctests run
  • Added linters to automatically check that docstrings are valid and up-to-date :) 🙌

@benjello regarding the style of docstrings specifically, do you have any preference? To resume my take on this:

  • There are four main styles: Google, NumPy, Sphinx, and PEP
  • We currently have all four in the code 😄 and no official guideline as to prefer one over the others
  • I like Google because is easier to write and read, and numpydoc could make sense because we use numpy a lot.

In any scenario, this changeset has little to no impact on that because we're adding/completing doc and doctests rather than rewriting it to a style or the other, but as we move on documenting the whole code base (are we?) it could make sense to have a collective community discussion to choose one style and adhere to it, as it'll make contributions and maintenance easier.

@benjello
Copy link
Member

@maukoquiroga : I like Google style too.

openfisca_core/commons/rates.py Outdated Show resolved Hide resolved
openfisca_core/commons/tests/test_rates.py Outdated Show resolved Hide resolved
openfisca_core/commons/tests/test_rates.py Outdated Show resolved Hide resolved
@bonjourmauko bonjourmauko changed the base branch from master to fix-doc August 31, 2021 11:02
Copy link
Member

@benjello benjello left a comment

Choose a reason for hiding this comment

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

See my comment.
My fear is the the current typing may force bool and int array conversion to float array.
Otherwise good to go.
Thanks !

openfisca_core/commons/typing.py Outdated Show resolved Hide resolved
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

The only blocking change for me is the addition of a test coverage threshold in a PR presented as improving doctests. I believe that such a change has a potential impact on contributions acceptance processes, and should be subject to a separate discussion 🙂

I'm glad to see we've made a lot progress and are very close to shipping, notably thanks to a direct discussion with @maukoquiroga yesterday. Thanks for that!

openfisca_tasks/lint.mk Outdated Show resolved Hide resolved
openfisca_tasks/lint.mk Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
setup.cfg Outdated Show resolved Hide resolved
Mauko Quiroga and others added 2 commits October 7, 2021 15:39
Co-authored-by: Matti Schneider <matti.schneider@beta.gouv.fr>
Copy link
Member

@MattiSG MattiSG left a comment

Choose a reason for hiding this comment

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

🚀 😭

@bonjourmauko bonjourmauko merged commit 29d13d1 into master Oct 7, 2021
@bonjourmauko bonjourmauko deleted the doctests/commons branch October 7, 2021 13:50
@bonjourmauko bonjourmauko added kind:test Adding missing tests or correcting existing tests and removed kind:refactor Refactoring and code cleanup labels Sep 30, 2024
@bonjourmauko bonjourmauko self-assigned this Sep 30, 2024
@bonjourmauko bonjourmauko changed the title [1/17] Improve commons module doctests test: improve commons module doctests Sep 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind:test Adding missing tests or correcting existing tests
Projects
Development

Successfully merging this pull request may close these issues.

Fix commons doctests
4 participants