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

Issue #58812 : DOC : Added doc string to announce.py and contibutors.py #58826

Closed
wants to merge 2 commits into from
Closed

Conversation

malhar2460
Copy link

@malhar2460 malhar2460 commented May 25, 2024

Copy link
Member

@Aloqeely Aloqeely left a comment

Choose a reason for hiding this comment

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

Amazing work! I think this is a bit overkill though, in my opinion only the summary should be kept because this is a command line script so we don't really need to explain how to use these functions. Or maybe don't include the examples section which takes a lot of space

@@ -57,6 +57,34 @@


def get_authors(revision_range):
"""
Extract authors from a range of revisions
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Extract authors from a range of revisions
Extract authors from a range of revisions.

Can you make sure there's a dot after every short summary?

Copy link
Author

Choose a reason for hiding this comment

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

Done 👍

@malhar2460
Copy link
Author

malhar2460 commented May 27, 2024

@Aloqeely Okay so should i remove the long summary or examples from doc string

@jbrockmendel
Copy link
Member

iirc this file is vendored so we wouldn’t want to edit it

@malhar2460
Copy link
Author

@jbrockmendel Sorry im new to this, can you give me some more detail about what to do, i didn't understand

@Aloqeely
Copy link
Member

iirc this file is vendored so we wouldn’t want to edit it

Ah, I didn't know that. After further search it seems most of the copied code is from scipy tools/authors.py which has "This script is in the public domain." written at the top of the file, so I think it's ok to edit it

@jbrockmendel
Copy link
Member

so I think it's ok to edit it

We can edit it, but generally don't. If someone thinks there is an improvement to be made in this file, it should be done upstream.

@malhar2460
Copy link
Author

@Aloqeely I have removed the examples from doc string and added full stop to the shot summary, is there any other change that needs to be made?

@Aloqeely
Copy link
Member

@malhar2460 I am sorry but as mentioned here, this file is vendored so it isn't touched very often and I don't think we're gonna update it just to add the docstrings. Really sorry for taking much of your time!

On a note I'm still OK with this going through since the functions here are different from the ones upstream, but leaving final decision for @jbrockmendel

@Aloqeely Aloqeely added the Docs label May 29, 2024
@malhar2460
Copy link
Author

Okay, np👍🏻

@malhar2460 malhar2460 closed this by deleting the head repository Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DOC: Docstrings missing from .py files in Sphinxext docs folder
3 participants