-
Notifications
You must be signed in to change notification settings - Fork 172
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
feat: Update populate_exec command to cater to upcoming variant changes #4469
base: master
Are you sure you want to change the base?
Conversation
fcec2e6
to
2a38d86
Compare
for variant in variants: | ||
product.update({'variant': variant}) | ||
output_dict = self.get_transformed_data(row, product) | ||
output_writer = self.write_csv_row(output_writer, output_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 would suggest creating a new function, let's say get_variants. In there, instead of if-else, have a variant list that gets values extended or appended based on the key. The code will be readable and easy to understand.
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.
updated
...apps/course_metadata/management/commands/tests/test_populate_executive_education_data_csv.py
Outdated
Show resolved
Hide resolved
03031eb
to
023bb42
Compare
023bb42
to
0b6c76d
Compare
variants = [] | ||
|
||
for key in variant_keys: | ||
if key in product: |
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.
if key in product
only checks the presence of key. An additional check for truthy value is needed here.
0b6c76d
to
9490bef
Compare
PROD-4206
This PR updates populate_exec command to cater to upcoming variant changes.
Variants data will be split up into multiple keys. The new format is as follows: