-
Notifications
You must be signed in to change notification settings - Fork 17
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
docs: [FC-0074] enrich docstrings following a template with more details #246
base: main
Are you sure you want to change the base?
Conversation
Thanks for the pull request, @mariajgrimaldi! This repository is currently maintained by @openedx/hooks-extension-framework. Once you've gone through the following steps feel free to tag them in a comment and let them know that your changes are ready for engineering review.
|
68f37c3
to
d17276b
Compare
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 generally it is better practice to separate commits from changes from commits with version upgrades, it looks like you've done it all in one commit. But I am not going to block on this feedback.
openedx_filters/learning/filters.py
Outdated
- message: error message for the exception. | ||
- response: custom response which will be returned by the account settings view. |
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.
Same as above
This filter is triggered when a user requests a certificate, just before the certificate is created allowing the | ||
filter to act on the user, course key, mode, status, grade, and generation mode. | ||
|
||
Usage: |
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.
Do you think we should add this Usage in the other filters?
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.
No, this was not supposed to be here. Thanks for noticing.
openedx_filters/learning/filters.py
Outdated
message: error message for the exception. | ||
response: custom response which will be returned by the XBlock render view. |
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.
message: error message for the exception. | |
response: custom response which will be returned by the XBlock render view. | |
- message (str): error message for the exception. | |
- response (Optional[HttpResponse]): custom response which will be returned by the XBlock render view. |
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 don't think we need this level of detail in the docstrings though, since we already have the typing.
openedx_filters/learning/filters.py
Outdated
message: error message for the exception. | ||
response: custom response which will be returned by the XBlock render view. | ||
""" | ||
def __init__(self, message: str, response: HttpResponse = None): |
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.
def __init__(self, message: str, response: HttpResponse = None): | |
def __init__(self, message: str, response: Optional[HttpResponse] = None): |
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 for this!
openedx_filters/learning/filters.py
Outdated
|
||
Arguments: | ||
course_key (CourseKey): The course key for which the home url is being requested. | ||
course_home_url (String): The url string for the course home | ||
course_home_url (String): The url string for the course home. |
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.
course_home_url (String): The url string for the course home. | |
- course_home_url (str): The url string for the course home. |
openedx_filters/learning/filters.py
Outdated
|
||
Arguments: | ||
serialized_courserun (dict): courserun data | ||
serialized_courserun (dict): courserun data. |
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.
serialized_courserun (dict): courserun data. | |
- serialized_courserun (dict): courserun data. |
105a7ac
to
35ffa69
Compare
Comment to reviewers: The type-hint annotations are for documentation only for now. In another PR, I'll add the additional mypy checks. So there will probably be more changes around this. Thanks for the patience! |
Description
This PR modifies the docstrings of all current filters by adding:
Supporting information
This PR addresses a concern raised by this PR removing hooks docs from edx-platform: openedx/edx-platform#35921 (comment), where the table referencing where each event was triggered from was dropped.
Testing instructions
You can review the changes here: https://docsopenedxorg--246.org.readthedocs.build/projects/openedx-filters/en/246/reference/filters.html
Deadline
None
Checklists
Check off if complete or not applicable:
Merge Checklist:
Post Merge:
finished.