Issue/26506 Provides correct desciption in docstring that get_indexer methods are not yet supported #26519
Conversation
… so will back out and follow that pattern
Originally had made a template from the documentation so a common basis could be used with the IntervalIndex adding a warning that the other methods were as yet unsupported. In the end because I am not clear on what effect putting a template in the core documentation dictionary I feel back to just following get_loc and forking the doc string.
| I followed the example from get_loc to adjust the documentation, I originally had created a template the each documentation strings used as a basis but was concerned how it would interact with the global documentation dictionary and how it was used. Let me know if you would like any changes. |
timgates42 changed the title | Oh wow - found the right way to do it in the Contributors guide my apologies for rushing into it without checking the great documentation - I will fix the PR to follow the recomendation in https://pandas-docs.github.io/pandas-docs-travis/development/contributing_docstring.html#sharing-docstrings |
Codecov Report
@@ Coverage Diff @@ ## master #26519 +/- ## ========================================== - Coverage 91.75% 91.74% -0.01% ========================================== Files 174 174 Lines 50665 50664 -1 ========================================== - Hits 46489 46484 -5 - Misses 4176 4180 +4
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@ ## master #26519 +/- ## =========================================== - Coverage 91.75% 41.69% -50.07% =========================================== Files 174 174 Lines 50665 50643 -22 =========================================== - Hits 46489 21115 -25374 - Misses 4176 29528 +25352
Continue to review full report at Codecov.
|
Following the notes in the contributors guide - have opted for using docstring substition (https://pandas-docs.github.io/pandas-docs-travis/development/contributing_docstring.html#sharing-docstrings).
gfyoung added Docs Indexing 
WillAyd left a comment
WillAyd left a comment There was a problem hiding this comment.
I think it would be easier to just append a Raises section to the doctoring for IntervalIndex
| cc @jschendel |
| Thanks - good suggestion will do |
As suggested by @WillAyd instead of altering the docstring using a global substitution just append a raises docstring section that warns about the parts not yet implemented.
| Thanks @WillAyd it was much easier - have modified it to append a raises section to the docstring as requested. |
Raises section must appear before Examples which complicates the use of just appending a doc section and instead must perform a substitution.
| Hello @timgates42! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found: There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻 Comment last updated at 2019-05-28 22:40:21 UTC |
| Ran into a complication with the plan as the Raises section must be before the Examples in the docstring so hopefully this Substitution technique is the appropriate answer. |
Avoid exceeding column count and maintain flake8 line overflow requirements
pandas/core/indexes/interval.py Outdated
| **{'raises_section': textwrap.dedent("""\ | ||
| Raises | ||
| ------ | ||
| raises NotImplementedError if any method other than than the default |
There was a problem hiding this comment.
Typically this is formatted like
Raises ------ ExceptionName Condition. There was a problem hiding this comment.
Thanks @TomAugspurger - have adjusted accordingly
If you supply an argument then it assumes the substitution is a straight sequence for the string format as it uses args or kwargs.
If we remove the line at the end when substituting in the raises section and add one at the start then in the template the raises_section substitution can be on its own line which is a cleaner solution - thanks to @jreback.
| @timgates42 I think this looks generally good. Can you post a screenshot of the rendered HTML doc? |
Sure @WillAyd here it is
|
| The Raises section looks off with the asterisks - do you know why that is appearing? …Sent from my iPhone On May 27, 2019, at 5:08 AM, Tim Gates ***@***.***> wrote: @timgates42 I think this looks generally good. Can you post a screenshot of the rendered HTML doc? Sure @WillAyd here it is — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread. |
| ahh, just noticed @WillAyd comment, can you update. |
| Good catch - according to the sphinx documentation this comes out if you are missing a colon (which I am) - I’ll add and regenerate - I actually copied this from another spot in pandas so presumably that needs fixing too - will find it |
| Oh interesting I found the function I copied rename_categories in pandas/core/arrays/categorical.py and the issue can be seen in the published doc http://pandas-docs.github.io/pandas-docs-travis/reference/api/pandas.CategoricalIndex.rename_categories.html?highlight=rename_categories#pandas.CategoricalIndex.rename_categories but the fix is not what I expected as pandas uses numpydoc a varient of sphinx doc and it looks like this raises syntax is common across pandas as is the strange asterisks sequence - http://pandas-docs.github.io/pandas-docs-travis/reference/api/pandas.api.types.union_categoricals.html?highlight=union_categoricals#pandas.api.types.union_categoricals |
| I do get these warnings when producing the documentation which is the next lead to track down... |
WillAyd left a comment
WillAyd left a comment There was a problem hiding this comment.
@timgates42 thanks for the research. Given this is a pre-existing issue across the code base I'm OK with this PR as is and would take a follow up PR to fix the asterisks.
Of course up to @jreback as well
| Oh just spotted I had doubled up than and it was |
without checking explicitly, it may be related to #26058, there seem to be a few issues regarding formatting. The dev docs at http://pandas-docs.github.io/pandas-docs-travis are being built on Travis using Sphinx v2.0.1 |
| Oh thanks for the heads up @simonjayhawkins I can confirm I built locally using sphinx v2.0.1 let me check 1.8.5 to see if it looks better. |
| Rolling back to sphinx 1.8.5 didn't solve the issue, I'll continue investigation. I've setup a separate repo at https://github.com/timgates42/numpydoc-demo to run some testing of numpydoc to see if I can reproduce the issue. If I solve it then I'm happy to send another PR as its unrelated to the problem being addressed here. Let me know if anyone disagrees or wants a change to this PR otherwise assuming all is good please merge it if this is appropriate - Thanks. |
| thanks @timgates42 if you can create an issue for the astericks thing (you can just refer back to the comments here), and a PR to fix would be great! |
7 participants
git diff upstream/master -u -- "*.py" | flake8 --diffProvides correct desciption in docstring that Interval.get_indexer using any method other than default are not yet supported