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

Reformat docstrings to Google style and add type annotations #3694

Merged
merged 42 commits into from
Mar 22, 2024

Conversation

DanielYang59
Copy link
Contributor

@DanielYang59 DanielYang59 commented Mar 18, 2024

Reformat docstrings to Google style and add type annotations

There should be no functional changes. Any change made to the code is to resolve mypy type errors.

Tasks:

  • Corrected some already Google styled docstrings. Replaced keywords Arg/Return/Raise with its plural form Args/Returns/Raises even when there is only one Arg/Return/Raise. Reference.
  • Replaced reStructuredText format by searching for keywords: :param, :type, :returns, :rtype, ::.
  • Did not find Scipy/Numpy/Epytext styled docstrings.

@DanielYang59

This comment was marked as off-topic.

tasks.py Show resolved Hide resolved
Signed-off-by: Haoyu (Daniel) <yanghaoyu97@outlook.com>
@DanielYang59 DanielYang59 changed the title Reformat docstrings to Google style Reformat docstrings to Google style and add type annotations Mar 19, 2024
@DanielYang59
Copy link
Contributor Author

DanielYang59 commented Mar 19, 2024

I guess it's not a good idea to reformat docstring and add type annotations (for modules I'm not quite familiar with especially) at the same time. I should focus on reformatting docstring for this PR and stop adding type annotations.

I tried my best to fix most mypy error but still get quite some left (some even seems like programming bugs).

I pinged a few I'm having trouble with. Could you please give me a hand when you have time? @janosh. Thanks a ton in advance. 😄

@janosh
Copy link
Member

janosh commented Mar 22, 2024

thanks so much @DanielYang59! this looks like a lot of work! mypy is happy now 26a2c8c :)

@janosh janosh added housekeeping Moving around or cleaning up old code/files docs Documentation, examples, user guides types Type all the things labels Mar 22, 2024
@DanielYang59 DanielYang59 marked this pull request as ready for review March 22, 2024 09:23
tasks.py Show resolved Hide resolved
Copy link
Member

@janosh janosh left a comment

Choose a reason for hiding this comment

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

thanks a lot @DanielYang59! 👍

@janosh janosh merged commit 6d16880 into materialsproject:master Mar 22, 2024
22 checks passed
@@ -82,7 +82,7 @@ def __init__(
if edges is None:
self.mol_graph = MoleculeGraph.with_local_env_strategy(molecule, OpenBabelNN())
else:
_edges = {(edge[0], edge[1]): None for edge in edges}
_edges: dict[tuple[int, int], dict | None] = {(edge[0], edge[1]): None for edge in edges}
Copy link
Contributor Author

@DanielYang59 DanielYang59 Mar 22, 2024

Choose a reason for hiding this comment

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

This looks like magic to me. Didn't expect declaring the super set of its actual type would solve the problem.

@DanielYang59
Copy link
Contributor Author

thanks a lot @DanielYang59! 👍

Thanks for reviewing! Glad I can help.

@DanielYang59 DanielYang59 deleted the format-docstr-google branch March 22, 2024 11:17
@DanielYang59
Copy link
Contributor Author

Ahh. I forgot to mention this but it seems we need to rebuild the docs after so many changes? @janosh

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Documentation, examples, user guides housekeeping Moving around or cleaning up old code/files types Type all the things
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants