BUG: Placeholders not being filled on docstrings #39139
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
MicaelJarniac
commented
Jan 13, 2021
•
MicaelJarniac
commented
- closes Documentation showing placeholders instead of text #39115
- tests added / passed
- Ensure all linting tests pass, see here for how to run them
|
It's my first time actually working with the code on this repo. I've contributed previously, but only with minor changes to documentation, and I've always done that from the GitHub built-in editor. I believe I've done everything correctly, but I'm completely unfamiliar with the codebase here. I've built the documentation locally and looked around to see if there was anything wrong, and it didn't seem like it. I didn't expect I'd be able to even find what was causing the bug I reported, let alone fix it, and I can't help but feel like I'm missing something important. Anyways, the fix I've put together is really simple, but I fear it might not be exactly how the maintainers would like it to be fixed, so if needed I can try to come up with something else to mitigate this issue. What I mean by "not how the maintainers would like it fixed" is that, for this fix to work, an empty This was the simplest way I found for fixing this, and in my opinion it has a nice pattern to it, but I do think there are other ways of fixing this. |
jreback
requested changes
Jan 13, 2021
I did run all tests I managed to find. I've run I'll run some more tests, and see if I can find out what those warnings are about. Edit: I forgot to mention, I've just forked this repo yesterday, and I did |
try to actually build the docs (these are not tests) |
|
I did build them, it took ages but worked fine. |
|
I've also shared a screenshot of the specific doc I was aiming to fix: |
|
I've merged the current |
|
I'll run the tests on the |
|
I found one instance where the I've now modified the fix slightly as to prevent this. Before: decorated.__doc__ = "".join(
[
component.format(**params)
if isinstance(component, str) and params != {}
else dedent(component.__doc__ or "")
for component in docstring_components
]After: decorated.__doc__ = "".join(
[
(component.format(**params) if params != {} else component)
if isinstance(component, str)
else dedent(component.__doc__ or "")
for component in docstring_components
]So now, even if an empty What was happening before is that, since The reason there's this check before using On this specific docstring, But with the docstring I've been working to fix, I've converted this PR to a draft because I still have to re-run all tests and rebuild the docs since I changed the code again. |
|
Okay, I've run all tests again, everything seems normal. Here's the result of And on the So yeah, there are 10 tests failing, but those tests were failing on The docs built correctly, and from my inspections everything was in order. I've also run all And I also compared the output of - /home/micael/projects/pandas-MicaelJarniac/pandas/core/generic.py:#L#:SS03:pandas.DataFrame.mad:Summary does not end with a period
- None:None:SS03:pandas.core.groupby.DataFrameGroupBy.mad:Summary does not end with a period
- /home/micael/projects/pandas-MicaelJarniac/pandas/core/generic.py:#L#:SS03:pandas.Series.mad:Summary does not end with a period |
|
I've merged On New branch:
|
|
I was thinking about adding tests to catch if I'm not sure if there's any instance where those are actually supposed to show up like that, but if there are, I don't think they'd be many, so I guess it'd be possible to whitelist (allowlist? passlist?) those specific instances, while having the checks on everything else. |
|
Thanks @MicaelJarniac for the PR. I think the solution should perhaps be more like... diff --git a/pandas/util/_decorators.py b/pandas/util/_decorators.py
index d002e8a4eb..91a55d1bf8 100644
--- a/pandas/util/_decorators.py
+++ b/pandas/util/_decorators.py
@@ -372,14 +372,17 @@ def doc(*docstrings: Union[str, Callable], **params) -> Callable[[F], F]:
docstring_components.append(docstring)
# formatting templates and concatenating docstring
- decorated.__doc__ = "".join(
+ docstring = "".join(
[
- component.format(**params)
+ component
if isinstance(component, str)
else dedent(component.__doc__ or "")
for component in docstring_components
]
)
+ if params:
+ docstring = docstring.format(**params)
+ decorated.__doc__ = docstring
# error: "F" has no attribute "_docstring_components"
decorated._docstring_components = ( # type: ignore[attr-defined]Then we don't need to apply the additional doc decorator while also using the doc decorator to inherit docstrings, e.g. when no parameters are specified and the inherited docstring contains curly braces, such as to prevent |
|
@MicaelJarniac a bit of history. we have @appender and @substition decorators, 177 and 96 usages repspectively. the @doc decorator was intended to be a replacement for these, currently 189 usages. The @doc decorator does seem to be causing issues with partial templating and where curly braces appear in the docstring. I would suspect that any changes to the @doc decorator could cause issues elsewhere and I would be reticent to backport any changes here to fix the broken docs. An alternative could be to use the legacy @appender and @substition to fix the DataFrame.mad and Series.mad docstrings for now. |
|
IIRC it is also possible for use instead of to get around the bug. so that would be easier. |
|
I'll look into those, that does sound like the safest approach for the short term. And as for the long term, I'll try to think of a way for using the For example, something like: for placeholder, value in params.items():
docstring = re.sub("{" + placeholder + "}", value, docstring)Something like that would mean that only placeholders specifically provided with |
|
@MicaelJarniac we have a release planned for Monday, so updated this to just fix the Series.mad and DataFrame.mad docstrings for now |
|
and Series.mad |
3 tasks
jreback
approved these changes
Jan 17, 2021
|
looks ok to me. @simonjayhawkins merge if good; we need a longer term soln right? |
all the other methods in |
meeseeksmachine
pushed a commit
to meeseeksmachine/pandas
that referenced
this pull request
Jan 17, 2021
|
Thanks @MicaelJarniac feel free to open another PR with the changes to the @doc decorator that you originally had. |
simonjayhawkins
pushed a commit
that referenced
this pull request
Jan 17, 2021
…39231) Co-authored-by: Micael Jarniac <[email protected]>
luckyvs1
pushed a commit
to luckyvs1/pandas
that referenced
this pull request
Jan 20, 2021
nofarm3
pushed a commit
to nofarm3/pandas
that referenced
this pull request
Jan 21, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.