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

Fix E731 error for sorters.py #1099

Merged
merged 7 commits into from
Jul 9, 2024
Merged

Conversation

stevenhua0320
Copy link
Contributor

No description provided.

Copy link
Contributor

@sbillinge sbillinge left a 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.

Convert a Datetime object of the doc to a float key.
Parameters
----------
x
Copy link
Contributor

@sbillinge sbillinge Jul 9, 2024

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
Copy link
Contributor

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
------------


Returns
-------
float representing the date key.
Copy link
Contributor

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

Copy link
Contributor

@sbillinge sbillinge left a 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.

Copy link
Contributor

@sbillinge sbillinge left a 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.

Parameters
----------
date: dict
the document that is expected to contain date-like information in "year" and/or "month"
Copy link
Contributor

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"


Parameters
----------
category : dict
Copy link
Contributor

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.


Returns
-------
serialization of string representing the category.
Copy link
Contributor

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.

@sbillinge sbillinge merged commit 1111f7b into regro:main Jul 9, 2024
1 check passed
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