-
Notifications
You must be signed in to change notification settings - Fork 52
fix: normalize enable_course_sorting_by_start_date to boolean type #239
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: normalize enable_course_sorting_by_start_date to boolean type #239
Conversation
|
Thanks for the pull request, @PKulkoRaccoonGang! This repository is currently maintained by 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. 🔘 Get product approvalIf you haven't already, check this list to see if your contribution needs to go through the product review process.
🔘 Provide contextTo help your reviewers and other members of the community understand the purpose and larger context of your changes, feel free to add as much of the following information to the PR description as you can:
🔘 Get a green buildIf one or more checks are failing, continue working on your changes until this is no longer the case and your build turns green. DetailsWhere can I find more information?If you'd like to get more details on all aspects of the review process for open source pull requests (OSPRs), check out the following resources: When can I expect my changes to be merged?Our goal is to get community contributions seen and reviewed as efficiently as possible. However, the amount of time that it takes to review and merge a PR can vary significantly based on factors such as:
💡 As a result it may take up to several weeks or months to complete a review and merge your PR. |
b89d948 to
a59d7ab
Compare
| return (self._end_time - self._start_time).seconds | ||
|
|
||
|
|
||
| def normalize_bool(value): |
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.
[inform]: Since distutils.util.strtobool is deprecated, a custom normalize_bool helper has been added.
f8ea35e to
42aca6a
Compare
42aca6a to
76cebd5
Compare
c447fa0 to
dbbbf9e
Compare
kyrylo-kh
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
bradenmacdonald
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.
Makes sense. Too bad the API is using POST form values instead of JSON data.
Description
Since we have the feature flag
ENABLE_COURSE_SORTING_BY_START_DATE, which provides a boolean value, and we also receive theenable_course_sorting_by_start_dateparameter (as a string) through the form-data in the/search/unstable/v0/course_list_search/API, this PR introduces thenormalize_boolfunction to ensure that theenable_course_sorting_by_start_dateparameter is always a boolean before being used in conditional logic.Before
The value of
enable_course_sorting_by_start_datewas always evaluated asTrue, because it was passed as a string ("true" / "false").Now
Depending on the actual data type of
enable_course_sorting_by_start_date, its value is normalized to a proper Python bool using the newnormalize_boolfunction.