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

Fixed most See Also docstring formatting, quietened the last warnings coming from doctests #3932

Merged
merged 3 commits into from
Jul 9, 2022
Merged

Fixed most See Also docstring formatting, quietened the last warnings coming from doctests #3932

merged 3 commits into from
Jul 9, 2022

Conversation

alexander-beedie
Copy link
Collaborator

@alexander-beedie alexander-beedie commented Jul 7, 2022

  • The formatting for most of the See Also subsections in docstrings was a bit off, so they weren't being rendered properly on the Sphinx Polars API website. This should fix them. Will be worth adding some more of these links/refs later.

  • Also, found the last two or three doctests warnings and took care of them - everything there passes cleanly.

  • As they now run clean, I added doctests to the recently introduced make test-all option in the py-polars Makefile; this is not currently run automatically anywhere, it's manual/opt-in (as it runs both tests and parametric_tests), but as I have been bitten a couple of times by doctests not passing on commit, it will help prevent a few of those before they hit GitHub... ;)

  • Reduced the default scale of parametric tests to speed them up a touch.

…st-all" (and they now pass with no warnings/errors)
@github-actions github-actions bot added the python Related to Python Polars label Jul 7, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #3932 (2209ebf) into master (0291463) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #3932      +/-   ##
==========================================
+ Coverage   62.42%   62.45%   +0.02%     
==========================================
  Files         447      447              
  Lines       73773    73773              
==========================================
+ Hits        46054    46075      +21     
+ Misses      27719    27698      -21     
Impacted Files Coverage Δ
py-polars/polars/internals/expr.py 94.54% <ø> (ø)
py-polars/polars/internals/frame.py 93.19% <ø> (ø)
py-polars/polars/internals/lazy_frame.py 75.68% <ø> (ø)
py-polars/polars/internals/series.py 94.14% <ø> (ø)
py-polars/polars/internals/whenthen.py 94.73% <ø> (ø)
py-polars/polars/testing.py 92.65% <100.00%> (-0.57%) ⬇️
...s/polars-core/src/chunked_array/ops/take/traits.rs 51.61% <0.00%> (-1.08%) ⬇️
polars/polars-core/src/series/series_trait.rs 12.05% <0.00%> (-1.07%) ⬇️
polars/polars-arrow/src/kernels/take.rs 70.12% <0.00%> (-0.25%) ⬇️
... and 8 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0291463...2209ebf. Read the comment docs.

@ghuls
Copy link
Collaborator

ghuls commented Jul 7, 2022

Can we have an empty newline after - - - - - - lines (like example) ? Without is fine when rendering html, but I use quite often ?pl.Expr.head to show help in an interactive interpreter.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Jul 8, 2022

Can we have an empty newline after - - - - - - lines (like example) ? Without is fine when rendering html, but I use quite often ?pl.Expr.head to show help in an interactive interpreter.

Odd; looked like a formatting inconsistency. Which interpreter/env are you using? I see no issue here with/without a newline (in PyCharm, with/without ipython, using either help(xyz) or xyz?). Do the examples go missing under your interpreter without the leading newline?

There is currently no real consistency in the codebase as to whether or not docstring subsections have a newline beneath the "---", so if there's a reason to choose one format over the other it's probably worth standardising.

@ritchie46
Copy link
Member

I agree it is a good custom to end with a new line. I have requested people to do this with the examples as well.

If I could I would check this in CI so that we can enforce that.

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Jul 8, 2022

I agree it is a good custom to end with a new line. I have requested people to do this with the examples as well.

They end with a newline - this is about adding a newline after (all?) the section underscores:

Usual:

Parameters
----------
x
y
z

Examples
--------
>>> do_something()

With extra newlines:

Parameters
----------
                     ## extra newline
x
y
z

Examples
--------
                     ## extra newline
>>> do_something()

Is there a particular advantage to the latter? (I've not seen this style of docstring spaced-out like that before, so wondering if it's an editor-specific thing). I have no strong opinion either way, and am generally in favour of standardising.

@ritchie46
Copy link
Member

Is there a particular advantage to the latter? (I've not seen this style of docstring spaced-out like that before, so wondering if it's an editor-specific thing). I have no strong opinion either way, and am generally in favour of standardising.

Right, sorry, I misunderstood. Let's take numpydoc as guideline then: https://numpydoc.readthedocs.io/en/latest/example.html

They don't have it, so let us not do that as well.

@ghuls
Copy link
Collaborator

ghuls commented Jul 8, 2022

ipython in a normal terminal (no notebook):

In [4]:  ?pl.Expr.pow
Signature: pl.Expr.pow(self, exponent: 'int | float | pli.Series | Expr') -> 'Expr'
Docstring:
Raise expression to the power of exponent.
Examples
--------

>>> df = pl.DataFrame({"foo": [1, 2, 3, 4]})
>>> df.select(pl.col("foo").pow(3))
shape: (4, 1)
┌──────┐
│ foo  │
│ ---  │
│ f64  │
╞══════╡
│ 1.0  │
├╌╌╌╌╌╌┤
│ 8.0  │
├╌╌╌╌╌╌┤
│ 27.0 │
├╌╌╌╌╌╌┤
│ 64.0 │
└──────┘
File:      ~/software/polars/py-polars/polars/internals/expr.py
Type:      function

@ghuls
Copy link
Collaborator

ghuls commented Jul 8, 2022

At least if the docstring formatting is consistent in all samples.

@alexander-beedie
Copy link
Collaborator Author

I suspect a suitable regex can normalise everything - I'll take a look.

@ritchie46
Copy link
Member

Is it good to go @alexander-beedie ?

@alexander-beedie
Copy link
Collaborator Author

alexander-beedie commented Jul 8, 2022

Is it good to go @alexander-beedie ?

@ritchie46: Yup, good to go; there weren't actually that many newlines to normalise in the end - think I got most (all?) of them. Can do a second-pass If anyone spots something else that would benefit from standardising.

thatlittleboy added a commit to thatlittleboy/polars that referenced this pull request Jul 9, 2022
@ritchie46 ritchie46 merged commit e4371fa into pola-rs:master Jul 9, 2022
@alexander-beedie alexander-beedie deleted the docstrings-and-doctests branch July 9, 2022 06:49
thatlittleboy added a commit to thatlittleboy/polars that referenced this pull request Jul 11, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
python Related to Python Polars
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants