-
Notifications
You must be signed in to change notification settings - Fork 71
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
Fix E731 error for sorters.py #1099
Conversation
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.
please see my inline comments and try and update all the functions using this as a template.
regolith/sorters.py
Outdated
Convert a Datetime object of the doc to a float key. | ||
Parameters | ||
---------- | ||
x |
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.
let's give this a more intuitive name like document
then in the numpy-style docstring it would look like
document: dict
the document that is expected to contain date-like information in "year" and/or "month"
def doc_date_key(x): | ||
""" | ||
Convert a Datetime object of the doc to a float key. | ||
Parameters |
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.
we need a blank line here in all the docstrings, and the first line inline, so something like:
"""Convert a Datetime object of the doc to a float key.
Parameters
------------
regolith/sorters.py
Outdated
|
||
Returns | ||
------- | ||
float representing the date key. |
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.
I think there is no key here, maybe more descriptive would be the float serialization of the date information in the document
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.
sorry for this, but please see my edited comment. I read the code more carefully and I realize that the thing I called "date" is not a date but is a document, so let's call it a document. My suggested docstring is in the edited 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.
please see additional comments inline.
regolith/sorters.py
Outdated
Parameters | ||
---------- | ||
date: dict | ||
the document that is expected to contain date-like information in "year" and/or "month" |
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.
this should be "end_year"
and "end_month"
regolith/sorters.py
Outdated
|
||
Parameters | ||
---------- | ||
category : dict |
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.
I think the input should again be called document here.
regolith/sorters.py
Outdated
|
||
Returns | ||
------- | ||
serialization of string representing the category. |
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.
this is not a serialization, it is just returning the category item. We probably don't need this function in fact. Like elsewhere, we could just put the lambda inside the sorter, unless it is used in many places. For now, just make changes to the docstring and let's go with that.
No description provided.