ddionrails / collect_stata

Accumulate data from stata files and write it into an open format
BSD 3-Clause "New" or "Revised" License
0 stars 1 forks source link

Improve Docstrings/Readability #18

Open hansendx opened 5 years ago

hansendx commented 5 years ago

Many off the docstrings are not helpful. Please elaborate on why a function exists and why it works how it works. For the most part, your code should describe the 'what', the comments the 'why'. If the what is not clear from the code, please elaborate with comments. Please format your docstrings after google's style guide.

afuetterer commented 5 years ago

I would like to see type hints like in this example:

def function_with_pep484_type_annotations(param1: int, param2: str) -> bool:
    """Example function with PEP 484 type annotations.

    Args:
        param1: The first parameter.
        param2: The second parameter.

    Returns:
        The return value. True for success, False otherwise.

    """

Reference: https://sphinxcontrib-napoleon.readthedocs.io/en/latest/example_google.html

afuetterer commented 5 years ago

I found a mistake in one of your docstings:

https://github.com/ddionrails/collect_stata/blob/54f33d3d11e0b4e6ba1c4f989c56e293258c9b82/collect_stata/dataset.py#L47

This should not be *.html.

afuetterer commented 5 years ago

Also how do you "test" data? And there is only the JSON export now? https://github.com/ddionrails/collect_stata/blob/54f33d3d11e0b4e6ba1c4f989c56e293258c9b82/collect_stata/dataset.py#L10

Please update the docstring.

afuetterer commented 5 years ago

There is no csv file being written. https://github.com/ddionrails/collect_stata/blob/54f33d3d11e0b4e6ba1c4f989c56e293258c9b82/collect_stata/stata_to_json.py#L15

afuetterer commented 5 years ago

The function does not return an OrderedDict. https://github.com/ddionrails/collect_stata/blob/54f33d3d11e0b4e6ba1c4f989c56e293258c9b82/collect_stata/write_json.py#L259-L262

Please consider using type hints, as they can give you help in finding mistakes like this.

def generate_stat(data, metadata, study) -> OrderedDict:
    return []

This should be marked as being typed wrong, when the function returns in fact a list instead of a OrderedDict.