-
Notifications
You must be signed in to change notification settings - Fork 0
Add Type Checking, Exports, Enum Config File, Generators/PV #3
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
Conversation
sarahmish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @amzhao16! Some feedback I have is:
- Remove any
__pycache__files from this PR - Please apply the docstrings comment to all docstrings, not just the one I highlighted.
- Make sure that all arguments of the function have a corresponding definition in the docstrings.
- For all public functions, make sure they have docstrings.
- To make functions private, i.e. not exposed to the user such as checking methods, you can prefix the function with
_, e.g._get_enum_obj. - Standardize the arguments on whether or not you use typing, especially for public functions.
pygridsim/core.py
Outdated
| """ | ||
| When the user wants to manually add nodes, or make nodes with varying parameters. | ||
|
|
||
| Args: | ||
| params: load parameters for these manual additions | ||
| load_type: input as string, representing one of the load types | ||
| lines: which nodes these new loads are connected to | ||
| num (optional): number of loads to create with these parameters | ||
| Return: | ||
| List of load_nodes | ||
| """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A couple of things should be considered regarding the docstrings:
- Each argument should have an arg_type as well as a description. I recommend to put the argument description in a line below the argument name instead of putting all of it in the same line. This ensures that format is properly parsed when documentation is rendered.
- A
Returnsblock should be added listing the return type and a description in the next line. - A blank line should always exist between the first line and the rest, and also before
ArgsandReturns.
"""Short description in a single line ending with a dot.
Longer description that can span across multiple lines. Longer
description that can span across multiple lines. Longer description
that can span across multiple lines.
Args:
arg_name (arg_type):
argument description.
arg_name (arg_type):
Argument description that spans across multiple lines. Argument
description that spans across multiple lines. Argument description
that spans across multiple lines.
Returns:
return_type:
description of the returned objects
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Making these changes for the public functions. For private functions (like _make_source_node, get_param) that I don't want the user to access, what do you recommend? Should I still follow docstring convention, removing the docstring entirely, or doing a different type of docstring? Thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for private functions, no docstrings are needed. make sure to indicate that they are private by prefixing them with _.
In addition, all docstrings should follow the same convention (preferably what I have indicated above).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, the docstrings in the most recent commit should follow the convention you indicated. I will now remove docstrings from the private functions
|
…tions, remove mutable types from function argument, etc
sarahmish
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM @amzhao16 – double check that sim.json is needed in this PR. Other than that, I think it's good to go!
Sorry for making this a large pull request, this contains most of my work over the past 1-2 weeks but I will try to detail the main changes below. I will also be branching off of this branch for some remaining fixes later this week.