-
Notifications
You must be signed in to change notification settings - Fork 8
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
Modular import/export taxonomy tags #64
Modular import/export taxonomy tags #64
Conversation
Thanks for the pull request, @ChrisChV! Please note that it may take us up to several weeks or months to complete a review and merge your PR. Feel free to add as much of the following information to the ticket as you can:
All technical communication about the code itself will be done via the GitHub pull request interface. As a reminder, our process documentation is here. Please let us know once your PR is ready for our review and all tests are green. |
4b9d584
to
b056500
Compare
return str(error) | ||
|
||
# Or return the plan | ||
return dsl.plan() |
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.
Nit: It makes the type inference less useful if a function returns different data types in different situations... here it can return errors, a bool (from .plan()) or a plan. So if mypy
is working correctly, you technically have to handle all three cases every time you call this function to avoid a type warning.
Instead, I'd recommend splitting plan() and execute() into separate functions, or making this combined one so that it always return a tuple like (plan: TagImportDSL, executed: bool)
. For errors you can raise them as an exception instead of an alternate return 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.
Updated to only return a boolean (Ref).
- Added Parser, JSONParser and CSVParser classes to validate file and tags format - Tests added
1aa130e
to
4dc7c6f
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.
Hi @ChrisChV! I found only some typos in the code and one suggestion (feel free to disagree!).
I read the code and ran the tests locally.
Read a .json file and validates the root structure of the json | ||
""" | ||
file.seek(0) | ||
tags_data = json.load(file) |
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.
Is it worth checking if the input is a valid JSON to avoid an exception? Will someone up in the stack call stack handle this?
tags_data = json.load(file) | |
try: | |
tags_data = json.load(file) | |
except json.JSONDecodeError as e: | |
return None, [ | |
InvalidFormat( | |
tag=None, | |
format=cls.format.value, | |
message=str(e) | |
) | |
] | |
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.
Any errors occurred during the import are handled and logged in the import task. But yes, I agree with you that this verification should be more explicit.
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 here: 1577938
|
||
# Checks that exists only one tag | ||
if not _check_unique_import_task(taxonomy): | ||
raise ValueError( |
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.
In the future, we may need a function to force clean the status of an import task to avoid a lock. Maybe we end up with an invalid state (a task not in SUCCESS nor ERROR) caused by a server crash or an unforeseen condition that prevents us from doing further imports.
I don't think this is a problem for the MVP.
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.
Good catch! I added a list of TODOs on the docstring with this case
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.
Hey, this is very nice! Some comments/questions but I hope they're fairly minor.
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.
Nice work 😎
@ChrisChV 🎉 Your pull request was merged! Please take a moment to answer a two question survey so we can improve your experience in the future. |
Description
Implements import_tags and export_tags functions
A modular implementation has been followed for both functionalities. You can see more about this here
Supporting information
Closes openedx/modular-learning#64
Testing instructions
make requirements
Manual testing is possible but not very useful, since we don't yet have views for these models and APIs.
But you can check that the added tests cover the expected import actions and export outputs, and that tests are still covering 100% of the openedx_tagging module.