cryo icon indicating copy to clipboard operation
cryo copied to clipboard

WIP: Add docstring to freeze

Open 0xstubbs opened this issue 1 year ago • 1 comments

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

0xstubbs avatar Jan 10 '24 19:01 0xstubbs

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

sslivkoff avatar Jan 11 '24 02:01 sslivkoff