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

Consistently document parameter types #193

Closed
vyasr opened this issue Dec 12, 2019 · 11 comments
Closed

Consistently document parameter types #193

vyasr opened this issue Dec 12, 2019 · 11 comments
Labels
blocked Dependent on something else documentation Writing or editing documentation good first issue Good for newcomers

Comments

@vyasr
Copy link
Contributor

vyasr commented Dec 12, 2019

Documentation of functions should be consistent on including parameter types. Currently many functions that are part of the API (but are likely those that are rarely accessed by users) have parameter names and purposes documented, but are missing the :type ...: tag.

@bdice bdice added documentation Writing or editing documentation good first issue Good for newcomers labels Jan 30, 2020
@kidrahahjo
Copy link
Collaborator

@vyasr I'd like to document the API functions. Can you please assign me this?

@csadorf
Copy link
Contributor

csadorf commented Feb 18, 2020

@vyasr One thing that has not been discussed yet, since we are already going through all the functions, maybe we should use type hints to document the argument types?

@bdice
Copy link
Member

bdice commented Feb 18, 2020

@csadorf @vyasr I looked into adding type hints recently. The typing module was added in Python 3.5 but expanded/improved significantly in 3.6. Now that we have dropped 3.4, we could investigate this but we may run into limitations that are solved in 3.6 or newer. I think it would be good to try on one module in the same way that we’re looking at docstring linting starting with one file — perhaps we should consider doing both upgrades at the same time.

@vyasr
Copy link
Contributor Author

vyasr commented Feb 18, 2020

I like @bdice's solution to try it with one module and see what the limitations are. I don't know if the solutions for things like Generic types etc were added in 3.5 or 3.6, so we can figure it out by trying. I would recommend doing type hints independently of the changes proposed in this PR. Based on our discussions at the most recent dev meetings, I would prefer to wait on this PR until after we make progress on glotzerlab/signac-docs#64, at least to the point of choosing a style (probably numpydoc) and reformatting the existing docstrings. I remember the autoconversion tools (pyment was the best that I found) being better than nothing but still requiring thorough review afterwards. As a result, I think it's better to add a bunch of new docstrings after the conversion has happened.

That being said, I propose the following action items (probably to be made into separate issues):

  1. Get Add Doc Linting to CI signac#294 figured out and merged in.
  2. Make a corresponding PR for signac-flow.
  3. Run pyment (or something similar) to convert all our docstrings on signac.
  4. Repeat for signac-flow.
  5. Revisit this issue.

Does that general process sound right? I can port some of this to the docs revamp issue as well. Applying type hints can be done independently because it does not modify docstrings.

@csadorf csadorf added the blocked Dependent on something else label Feb 18, 2020
@csadorf
Copy link
Contributor

csadorf commented Feb 18, 2020

I agree with @vyasr . Let's keep those two things separated and get the doc-strings sorted out first.

@kidrahahjo It seems like this issue is currently blocked. I think it's generally fine for you to work on this, but as suggested it's probably best to revisit this issue once signac#294 is resolved and leave it unassigned until then.

@kidrahahjo
Copy link
Collaborator

kidrahahjo commented Feb 18, 2020

@csadorf Okay, will keep an eye on signac#294 and adhere to resolve this issue once that resolves

@kidrahahjo
Copy link
Collaborator

@vyasr Do we only have to update (add parameter type) the API doc in the methods where param name are mentioned?

@vyasr
Copy link
Contributor Author

vyasr commented Mar 31, 2020

@kidrahahjo and I have discussed this offline, but I'll document the results here. Currently @CryoZEUS is working on #278, which is a prerequisite to addressing this issue. In the process of completing #278 I expect that the autoconversion tool will add some of the missing types in automatically. @kidrahahjo is going to work on setting up CI (#276) for documentation so that we can start testing the documentation once it's converted to numpy style. Once that's all done, I think the focus of this issue will shift from updating the docstrings (hopefully that is done in the process of completing #278 and #276) to adding type hints. glotzerlab/signac#313 provides a template for how we can do this throughout our code base.

@bdice
Copy link
Member

bdice commented Dec 10, 2020

PRs #387, #389, and #392 offer a significant improvement on the state of this issue, with pydocstyle and numpy style docstrings throughout the package.

I don't think it is likely that we will adopt type annotations for the package unless someone is volunteering to do that work. I would feel comfortable marking this issue as resolved, if others agree.

@vyasr
Copy link
Contributor Author

vyasr commented Dec 10, 2020

I think type annotations are out of scope for this particular issue based on the title and the prior discussions indicating that we should handle documentation and annotations separately. I would be fine resolving this issue, but I think it's worth opening a separate one for type annotations. Adding those piecemeal would be a good first issue for new contributors. If we're bothering to use mypy validation as part of our pre commit config, we might as well solicit help on adding more annotations, they definitely help in finding various bugs and frankly signac-flow APIs could use that kind of validation. If that issue goes super stale we can close it, but at least then it will be easy to find if someone decides to take it on and we need to reopen.

@csadorf
Copy link
Contributor

csadorf commented Dec 17, 2020

I agree, introducing type hints is a separate issue, good to close. I created #404 to keep track of that.

@csadorf csadorf closed this as completed Dec 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked Dependent on something else documentation Writing or editing documentation good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants