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.

Sign up for GitHub

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

Jump to bottom

Apply NumPy docstyle to remaining files in "signac" root. #343

Merged
merged 12 commits into from
Jul 6, 2020
Merged

Apply NumPy docstyle to remaining files in "signac" root. #343

merged 12 commits into from
Jul 6, 2020

Conversation

bdice
Copy link
Member

@bdice bdice commented Jun 29, 2020

Description

This PR applies NumPy docstyle to remaining files in the signac directory (i.e. the level of __init__.py).

Motivation and Context

Progress on #315. After this, remaining PRs should remove core, common, and contrib from the "skipped" directories in match-dir in setup.cfg.

Types of Changes

  • Documentation update
  • Bug fix
  • New feature
  • Breaking change1

1The change breaks (or has the potential to break) existing functionality.

Checklist:

If necessary:

  • I have updated the API documentation as part of the package doc-strings.
  • I have created a separate pull request to update the framework documentation on signac-docs and linked it here.
  • I have updated the changelog and added all related issue and pull request numbers for future reference (if applicable). See example below.

@bdice bdice self-assigned this Jun 29, 2020
@bdice bdice added the doc-style Update docs to numpy doc style label Jun 29, 2020
@bdice bdice added this to the v1.5.0 milestone Jun 29, 2020
@bdice bdice changed the base branch from master to numpy_docs June 29, 2020 04:45
@bdice bdice marked this pull request as ready for review June 29, 2020 04:45
@bdice bdice requested review from a team as code owners June 29, 2020 04:45
@bdice bdice requested review from mikemhenry and cbkerr and removed request for a team June 29, 2020 04:45
@bdice bdice mentioned this pull request Jun 29, 2020
Closed
cbkerr
cbkerr approved these changes Jun 29, 2020
View reviewed changes
Copy link
Member

@cbkerr cbkerr left a comment

Choose a reason for hiding this comment

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

Some optional arguments do not list types. I highlighted some of them with inline comments.
Additionally, should we explicitly say optional like suggested in numpydoc? x : int, optional

Otherwise, looks good.

signac/sync.py Outdated
exclude : str
A filename exclusion pattern. All files matching this pattern will be
excluded from the synchronization process. (Default value = None)
doc_sync :
Copy link
Member

Choose a reason for hiding this comment

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

Do doc_sync and strategy need a type?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea what to put for the type of doc_sync. It supports None, False, 'copy', and callables. Users are supposed to provide one of the properties/functors from the DocSync class, but I'm not sure how to express that as a type. @csadorf Do you have thoughts? Maybe this?

Suggested change
doc_sync :
doc_sync : attribute or callable provided by :py:class:`~signac.sync.DocSync`

Copy link
Member Author

Choose a reason for hiding this comment

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

I went ahead and committed a slightly edited version of that suggestion above.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that's fine, but maybe you can add an example on how to use this function as part of the general description? I think that would make it much easier for users to understand how to use this.

signac/sync.py Outdated Show resolved Hide resolved
signac/syncutil.py Outdated Show resolved Hide resolved
signac/syncutil.py Outdated Show resolved Hide resolved
@bdice
Copy link
Member Author

bdice commented Jul 6, 2020

@cbkerr Excellent suggestions, thanks for catching the things I missed. I agree we should adopt the use of the "optional" keyword, but I realize that it is missing in many many places. I will create a separate issue for that so that we remember to apply it everywhere once our docstrings have all been converted.

@bdice bdice mentioned this pull request Jul 6, 2020
Closed
@bdice bdice merged commit 5186bb2 into numpy_docs Jul 6, 2020
@bdice bdice deleted the numpy_docs_remaining branch July 6, 2020 12:21
mikemhenry pushed a commit that referenced this pull request Jul 22, 2020
@bdice @atravitz
@ac-optimus @vyasr @zhou-pj
NumPy docstring style (#357)
7086d89 
* numpydoc style(signac/contrib/schema.py) (#318)

* numpydoc style(signac/contrib/project.py) (#320)

* numpydoc style(signac/contrib/job.py)  (#319)

* numpydoc style(signac/contrib/linked_view.py, signac/contrib/utility.py, signac/core/utility.py) (#326)

* numpydoc style(signac/contrib/errors.py, signac/contrib/filesystems.py, signac/contrib/filterparser.py, signac/contrib/hashing.py) (#322)

* numpydoc style (signac/contrib/collection.py) (#329)

* Make core submodule adhere to numpydoc conventions. (#339)

* add a docstring for public module (#337)

* numpydoc style (signac/contrib/import_export.py) (#333)

* Apply NumPy docstyle to remaining files in "signac" root. (#343)

* Apply fixes from minirst. (#356)

* Enforce pydocstyle in core, common, contrib. (#354)

Co-authored-by: Bradley Dice <[email protected]>
Co-authored-by: Alyssa Travitz <[email protected]>
Co-authored-by: Abhavya Chandra <[email protected]>
Co-authored-by: vyasr <[email protected]>
Co-authored-by: Pengji <[email protected]>
Co-authored-by: Mike Henry <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csadorf csadorf csadorf approved these changes

@cbkerr cbkerr cbkerr approved these changes

@mikemhenry mikemhenry Awaiting requested review from mikemhenry

Assignees

@bdice bdice

Labels
doc-style Update docs to numpy doc style
Projects
None yet
Milestone
v1.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@bdice @csadorf @cbkerr