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

WIP: Add docstring to freeze #169

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

0xstubbs
Copy link

@0xstubbs 0xstubbs commented Jan 10, 2024

Motivation

There are a lot of questions about how to implement cryo_python. It seems that most people either don't know what paras to use or they have slightly incorrect syntax. There is a lot of documentation about cryo_cli but implementation in python is slightly different and it is tripping people up.

Solution

Adding a more descriptive docstring for cryo_python.freeze

  • Using NumPy styling for docstring
  • Added most params and their types
  • Added examples of how to format different block ranges
  • Added list of datatypes and dataset group names

Requested Feedback

I'm looking for:

  • advice/feedback/opinionated comments about the current implementation.

I think that I have all params listed but haven't added a type or description to all of them. I started looking at _cryo_rust.parse_kwargs() and saw that there were more params and I realized I was starting to go deeper and deeper... but I realized I need your feedback on what I'm implementing before I fall too far into this rabbit-hole.

Thanks in advance for taking a look and I look forward to reviewing feedback
@sslivkoff

- Using NumPy styling for docstring
- Added most params and their types
- Added examples of how to format different block ranges
- Added list of datatypes and dataset group names
@0xstubbs 0xstubbs marked this pull request as ready for review January 11, 2024 02:26
@sslivkoff
Copy link
Member

yea there is a lot of room for improvement on the current docstrings. I think this PR goes in the right direction but a couple things I would change

  • the functions have a very large number of parameters, so need to consider whether it's even worth listing all of them. the alternative is just listing some of them and then providing either a url to the full list or a separate way to print out the full list
  • the formatting of the parameters that are included should be more succinct. 1 line per parameter max. I've never been a fan of numpy/google style docstrings, it's too verbose for situations like these
  • the indenting on the dataset list looks a bit off? and do you think it would be good to include all datasets here or just a subset?
  • don't want to use black-style double quotes for the formatting. currently using ruff with single quote config'd, but haven't gotten around to adding that to pyproject.toml yet

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants