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

Convert all docstrings to numpy style #315

Closed
vyasr opened this issue Mar 25, 2020 · 19 comments · Fixed by #357
Closed

Convert all docstrings to numpy style #315

vyasr opened this issue Mar 25, 2020 · 19 comments · Fixed by #357
Labels
documentation Writing or editing documentation enhancement New feature or request
Milestone

Comments

@vyasr
Copy link
Contributor

vyasr commented Mar 25, 2020

As part of our overall docs overhaul (see glotzerlab/signac-docs#64), we want to convert our docstrings to numpy style (as decided in glotzerlab/signac-docs#74). The best automated tool I'm familiar with for this task is pyment. In addition to converting docstrings, it will also generate docstrings for functions, classes, etc that are missing docstrings entirely. However, the conversion will require significant manual review to ensure that all docstrings are converted correctly.

@vyasr vyasr added documentation Writing or editing documentation enhancement New feature or request labels Mar 25, 2020
@vyasr
Copy link
Contributor Author

vyasr commented Mar 30, 2020

@ac-optimus could you comment on this issue so that I can assign you?

@ac-optimus
Copy link
Contributor

hi, can I work on this issue?

@mikemhenry
Copy link
Collaborator

mikemhenry commented Jun 11, 2020

List of files that don't have pull requests:

Common

  • signac/common/configobj/__init__.py
  • signac/common/configobj/validate.py
  • signac/common/configobj/_version.py
  • signac/common/config.py
  • signac/common/connection.py
  • signac/common/crypt.py
  • signac/common/host.py
  • signac/common/__init__.py
  • signac/common/validate.py

Contrib

  • signac/contrib/filesystems.py
  • signac/contrib/import_export.py PR numpydoc style (signac/contrib/import_export.py) #333
  • signac/contrib/indexing.py
  • signac/contrib/__init__.py
  • signac/contrib/migration/__init__.py
  • signac/contrib/migration/v0_to_v1.py
  • signac/contrib/mpipool.py

Core

PR #339.

  • signac/core/attrdict.py
  • signac/core/dict_manager.py
  • signac/core/h5store.py
  • signac/core/__init__.py
  • signac/core/json.py
  • signac/core/synceddict.py

Other

  • benchmark.py
  • doc/conf.py
  • setup.py
  • signac/db/database.py
  • signac/db/__init__.py
  • signac/diff.py
  • signac/__init__.py
  • signac/__main__.py
  • signac/sync.py
  • signac/syncutil.py
  • signac/testing.py
  • signac/version.py
  • signac/warnings.py PR Fix doc strings for signac/warnings.py #337
  • .sync-zenodo-metadata.py

Files that are checked off have pull requests pending or have been merged.

@bdice
Copy link
Member

bdice commented Jun 11, 2020

@mikemhenry Fantastic! We can also cross out any files that are deprecated and add them to a "disallow" list for docstyle checks, in addition to our current "allow" list.

@mikemhenry
Copy link
Collaborator

mikemhenry commented Jun 11, 2020

I'm thinking any files that are not in signac/ can be probably dropped? Also any files that have "THIS MODULE IS DEPRECATED!"

@mikemhenry
Copy link
Collaborator

Okay I think I got all the files that we should ignore, if anyone wants to propose/suggest others just let me know!

@csadorf
Copy link
Contributor

csadorf commented Jun 12, 2020

I don't think we have to adapt the doc-strings of vendored packages such as the configobj package.

@vyasr
Copy link
Contributor Author

vyasr commented Jun 12, 2020

I agree, we should leave those as is.

@bdice
Copy link
Member

bdice commented Jun 12, 2020

I edited @mikemhenry's comment directly to fix the vendored list. Good call. We could split up the list into 4 PRs: common, contrib, core, and "other".

@mikemhenry
Copy link
Collaborator

Thanks @bdice do you mean split the list up for organization or do you mean that we have 4 PRs that cover all of those folders/files instead of a PR-per-file

@bdice
Copy link
Member

bdice commented Jun 12, 2020

@mikemhenry I was considering that as 4 "blocks" of work that could each be assigned to a specific person. The choice of one PR per "block" or one PR per file could be up to the assignee.

@mikemhenry
Copy link
Collaborator

Got it!

@mikemhenry
Copy link
Collaborator

mikemhenry commented Jun 12, 2020

Headings updated!

@bdice
Copy link
Member

bdice commented Jun 15, 2020

I did the core submodule in PR #339. We'll need to render all the docs at the end and make sure it looks good before merging the numpy_docs branch -- there have been a few things that I've review/approved/written just from looking at the Python code without looking at how it is rendered. I think this will be easiest to do all at the end.

@bdice bdice added this to the v1.5.0 milestone Jun 25, 2020
@bdice
Copy link
Member

bdice commented Jun 28, 2020

I finished up #333, which is waiting on review.

@bdice
Copy link
Member

bdice commented Jun 29, 2020

I created #343. After #343 is merged, remaining PRs should remove core, common, and contrib from the "skipped" directories in match-dir in setup.cfg.

@csadorf
Copy link
Contributor

csadorf commented Jun 29, 2020

I've removed @ac-optimus as assignee since he is currently not contributing to this issue.

@bdice
Copy link
Member

bdice commented Jul 9, 2020

I think we may be ready to merge numpy_docs to the main branch after #352. We'll need a final review of the file list above, but importantly, pydocstyle should pass on the whole code base (excluding deprecations, which are handled by the settings in setup.cfg) after #352 is merged. This will allow us to start enforcing docstring style in some fashion for other pull requests, particularly #336.

  1. The core submodule is being heavily rewritten in Improve Sync Data Structures #336, so I don't think it is a good use of developer hours to update the existing core docstrings to NumPy docstring style right now.

  2. The common submodule is low priority: it has many deprecated features, its functionality is almost strictly for internal purposes, and it is not in the public API documentation at all (there is one un-linked reference to a Config object).

  3. The contrib submodule has already had significant rewriting and appears to be completely adherent to NumPy docstring style with the exception of deprecated modules.

@bdice bdice mentioned this issue Jul 16, 2020
9 tasks
@bdice
Copy link
Member

bdice commented Jul 16, 2020

@glotzerlab/signac-committers I've opened PR #357, which will finish up much of the work that has been done for this issue. We'll need to thoroughly review the discussion on this issue. I would advocate for us to review #357 as it is, merge it (unless critical problems are found), and push all follow-up work to new issues/PRs.

Copying from a comment @b-butler made on #354: there are a couple methods with # noqa: D103 because I couldn't figure out what to write for their docstrings and wanted pydocstyle to pass without proper docstrings.

def password(value, *args, **kwargs): # noqa: D103
return value
def get_validator(): # noqa: D103
return Validator({
'mongodb_uri': mongodb_uri,
'password': password,
})

Also related to what I was saying above: the common submodule has many candidates for public API deprecations, in my estimation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Writing or editing documentation enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants