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

Fix tseries.offsets docstrings with missing period in the summary (set 1) #144

Open
bhuvanakundumani opened this issue Aug 21, 2019 · 11 comments
Labels

Comments

@bhuvanakundumani
Copy link
Contributor

Same as #132 but for the next docstrings:

pandas.tseries.offsets.DateOffset.normalize: Summary does not end with a period
pandas.tseries.offsets.BusinessDay.normalize: Summary does not end with a period
pandas.tseries.offsets.BusinessHour.normalize: Summary does not end with a period
pandas.tseries.offsets.CustomBusinessDay.normalize: Summary does not end with a period
pandas.tseries.offsets.CustomBusinessHour.normalize: Summary does not end with a period
pandas.tseries.offsets.MonthOffset.normalize: Summary does not end with a period
pandas.tseries.offsets.MonthEnd.normalize: Summary does not end with a period
pandas.tseries.offsets.MonthBegin.normalize: Summary does not end with a period
pandas.tseries.offsets.BusinessMonthEnd.normalize: Summary does not end with a period
pandas.tseries.offsets.BusinessMonthBegin.normalize: Summary does not end with a period
pandas.tseries.offsets.CustomBusinessMonthEnd.normalize: Summary does not end with a period
pandas.tseries.offsets.CustomBusinessMonthBegin.normalize: Summary does not end with a period
pandas.tseries.offsets.SemiMonthOffset.normalize: Summary does not end with a period
pandas.tseries.offsets.SemiMonthEnd.normalize: Summary does not end with a period
pandas.tseries.offsets.SemiMonthBegin.normalize: Summary does not end with a period
pandas.tseries.offsets.Week.normalize: Summary does not end with a period
pandas.tseries.offsets.WeekOfMonth.normalize: Summary does not end with a period
pandas.tseries.offsets.LastWeekOfMonth.normalize: Summary does not end with a period

@bhuvanakundumani bhuvanakundumani changed the title Fix tseries.offsets docstrings with missing period in the summary #1 Fix tseries.offsets docstrings with missing period in the summary Aug 21, 2019
@bhuvanakundumani bhuvanakundumani changed the title Fix tseries.offsets docstrings with missing period in the summary Fix tseries.offsets docstrings with missing period in the summary Aug 21, 2019
@bhuvanakundumani bhuvanakundumani changed the title Fix tseries.offsets docstrings with missing period in the summary Fix tseries.offsets docstrings with missing period in the summary (set 1) Aug 21, 2019
@bhuvanakundumani
Copy link
Contributor Author

@datapythonista - On checking with each of these, i realised there are no docstrings .
Please refer this screenshot
Screenshot 2019-08-21 at 9 48 13 AM

Why are we getting the error as 'Summary does not end with a period' when summary is not there.
Thanks

@datapythonista
Copy link
Member

It may be a bug in the script, feel free to open an issue and work on a PR.

@bhuvanakundumani
Copy link
Contributor Author

Thanks @datapythonista . will check the script and work on it.

@bhuvanakundumani
Copy link
Contributor Author

@datapythonista

I tried running the validate_strings.py. It has doc.summary as well as doc.extended_summary. Please refer this screenshot -
Screenshot 2019-08-28 at 6 20 51 AM

When i tried building the html - I got this output.
Screenshot 2019-08-28 at 6 24 01 AM

It does not have the doc summary and extended summary. Can you give me some pointers to fix this?
Thanks

@datapythonista
Copy link
Member

@bhuvanakundumani this is the problem you're dealing with: pandas-dev/pandas#27055

Would be great if you can take that issue and fix it.

@bhuvanakundumani
Copy link
Contributor Author

I will look into it.

@bhuvanakundumani
Copy link
Contributor Author

bhuvanakundumani commented Sep 3, 2019

@datapythonista - Have been trying validate_docstrings.py and understand that the following piece of code

for part in func_parts:
     obj = getattr(obj,part) 

is getting the value of attribute normalize (which is False by Default). This when it is passed to pydoc.getdoc is getting us the docstring of False.

Am kinda stuck here. Any pointers on how i should be proceeding with this? Thanks

@datapythonista
Copy link
Member

Thanks for looking into this @bhuvanakundumani. After that is executed, a Docstring object is created, and if you check this method, is the one that tries to find the actual object (and not wrappers of it):

https://github.com/pandas-dev/pandas/blob/master/scripts/validate_docstrings.py#L296

It's likely that the problem is in that function.

@bhuvanakundumani
Copy link
Contributor Author

@datapythonista - On inspecting the code here -
https://github.com/pandas-dev/pandas/blob/master/scripts/validate_docstrings.py#L296

I think the code in this snippet -

class Docstring:
    def __init__(self, name):
        self.name = name
        obj = self._load_obj(name)
        self.obj = obj
        self.code_obj = self._to_original_callable(obj)
        self.raw_doc = obj.__doc__ or ""
        self.clean_doc = pydoc.getdoc(obj)
        self.doc = NumpyDocString(self.clean_doc)

I think this line of code

        self.clean_doc = pydoc.getdoc(obj)

should be

self.clean_doc = pydoc.getdoc(self.code_obj)

I tried the change and it reports the error - "No summary found (a short summary in a single line should be " (SS01) for pandas.tseries.offsets instead of "Summary does not end with a period".

However , i want to make sure this fix is right ? Are there any other tests that i have to do to ensure that the change is not breaking something else?

Thanks

@datapythonista
Copy link
Member

I think the change you propose will cause that properties and wrapped objects lose their docstring for the validation.

Not sure if _to_original_callable is well tested, if it's not it's probably worth adding unit tests for some known cases before fixing this bug.

I think the change to fix it should happen inside _to_original_callable.

Another useful test when we fix this would be to generate the json report for all docstrings with validate_docstrings.py, before and after the fix, and review some of the docstrings where the detected errors are different.

@bhuvanakundumani
Copy link
Contributor Author

Thanks @datapythonista . Will look into the tests and get back to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants