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

Doctest #232

Merged
merged 40 commits into from
Oct 2, 2024
Merged

Doctest #232

merged 40 commits into from
Oct 2, 2024

Conversation

BalzaniEdoardo
Copy link
Collaborator

  • Added doctest to tox.ini run by pytest for checking module docstrings
  • Fixed example to pass the tests
  • Added mpl as dev dependency (needed by doctest to run examples)

Note: I needed to capture outputs of fits and plots because they have a repr that prints the memory address of the object while for doctest the output of the console must be matched exactly.

@BalzaniEdoardo
Copy link
Collaborator Author

Additionally, I am ignoring the fetch and _documentation_utils modules because one would require pooch as dev dependency and the other would require seaborn

@BalzaniEdoardo BalzaniEdoardo requested review from billbrod, gviejo and sjvenditto and removed request for billbrod September 27, 2024 20:24
@codecov-commenter
Copy link

codecov-commenter commented Sep 27, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.24%. Comparing base (6dcd891) to head (9b1bb78).

Additional details and impacted files
@@             Coverage Diff              @@
##           development     #232   +/-   ##
============================================
  Coverage        97.24%   97.24%           
============================================
  Files               20       20           
  Lines             1815     1815           
============================================
  Hits              1765     1765           
  Misses              50       50           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

As I'm going through this, I think I'm not quite understanding what doctests do. Can you add a brief description? That should probably live in CONTRIBUTING.md. You could write up when they're necessary (note that pyopensci is going to ask you to have them for every public-facing function, so I'd say that, even if it's currently aspirational), how to write them, and what they'll check.

  • I'm confused as to why in some places the doctests includes the expected output and sometimes in doesn't. (Compare e.g., TransformerBasis.set_params and TransformerBasis).
  • Why the changing of some docstrings from strings to raw string literals? (with the addition of r)? My understanding is that the main difference has to do with the treatment of \, but most of those docstrings seem to not have any?
  • Up to you of course, but I think it's fine to include pooch and seaborn as dev dependencies, so that we can run the docstring tests on the whole tree. The dev bundle is really just for our convenience -- no one has to install them.

@BalzaniEdoardo
Copy link
Collaborator Author

BalzaniEdoardo commented Oct 1, 2024

As I'm going through this, I think I'm not quite understanding what doctests do. Can you add a brief description? That should probably live in CONTRIBUTING.md. You could write up when they're necessary (note that pyopensci is going to ask you to have them for every public-facing function, so I'd say that, even if it's currently aspirational), how to write them, and what they'll check.

Added a section on doctest in the CONTRIBUTING.md

* I'm confused as to why in some places the doctests includes the expected output and sometimes in doesn't. (Compare e.g., `TransformerBasis.set_params` and `TransformerBasis`).

when you capture the output storing it into the variable, no out is displayed (example out = TransformerBasis(basis) doesn't expect any out), .

* Why the changing of some docstrings from strings to raw string literals? (with the addition of `r`)? My understanding is that the main difference has to do with the treatment of `\`, but most  of those docstrings seem to not have any?

I needed in some multiline examples but I might have used it even if not necessary. I'll check on a case by case way.

* Up to you of course, but I think it's fine to include pooch and seaborn as dev dependencies, so that we can run the docstring tests on the whole tree. The dev bundle is really just for our convenience -- no one has to install them.

I move both to dev.

Copy link
Member

@billbrod billbrod left a comment

Choose a reason for hiding this comment

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

Two notes:

  • CONTRIBUTING should tell people what to do if they'd like to contribute. The tone of your doctest section is "suggesting", but it should be more declarative. Something like: "when you add a public function or method, you will need to include an examples section. This should be formatted as a doctest, which allows pytest to run the code as part of our testing suite. If you do not capture the output (e.g., >>> x + y vs. >>> z = x + y), the next line will need to show the expected output (e.g., 3) and the doctests will ensure that output matches. See below for an example..." Similarly, should provide some guidance on when to include doctests in text files.
  • Currently, not running doctests on the text files. Is it possible to come up with a general enough command to include in the tox?

CONTRIBUTING.md Outdated Show resolved Hide resolved
Co-authored-by: William F. Broderick <[email protected]>
@BalzaniEdoardo
Copy link
Collaborator Author

I moved into an issue the idea of running doctests for all text files. I think it is true, but not in this PR because all examples needs to be reformatted!

@BalzaniEdoardo BalzaniEdoardo merged commit a2d5dcc into development Oct 2, 2024
11 checks passed
@BalzaniEdoardo BalzaniEdoardo deleted the doctest branch October 2, 2024 22:34
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.

4 participants