-
Notifications
You must be signed in to change notification settings - Fork 18
feat: lp_dump management command added #335
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
Conversation
|
Thanks for the pull request, @dwong2708! 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. |
df51e04 to
df952d7
Compare
ormsbee
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.
Thank you for taking a crack at this. I have some high level requests and minor wording suggestions.
| from datetime import datetime | ||
| from typing import Any, Dict | ||
|
|
||
| from tomlkit import comment, document, dumps, nl, table |
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.
Since tomlkit is new, you'll need to add it to requirements/base.in and then compile the requirements using make compile-requirements in the top level directory. Please be sure to add comments in base.in explaining generally what this is used for.
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 thought it was already added, but it wasn’t. Just added it now — thanks!
| from tomlkit.items import Table | ||
|
|
||
|
|
||
| class LpTomlFile(): |
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 asked for
lp_dumpbecause I thought "lp" would be easier to type from the command line, but when we're talking about class names, please expand it to "LearningPackage". - In Java, the convention is to lowercase abbreviations after the initial captial, e.g. "Toml". In Python, the convention is to keep it all caps, e.g.
TOMLDecodeError.
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.
Done
openedx_learning/apps/authoring/backup_restore/management/commands/lp_dump.py
Outdated
Show resolved
Hide resolved
| self.stdout.write(self.style.SUCCESS(message)) | ||
| except Exception as e: # pylint: disable=broad-exception-caught | ||
| message = f"Error on create the zip file {e}" | ||
| self.stdout.write(self.style.ERROR(message)) |
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.
This should go to self.stderr, not self.stdout. But you can also just use logging and log.exception to put the stacktrace in it.
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.
Done
| @@ -0,0 +1,46 @@ | |||
| """ | |||
| Utilities for backup and restore app | |||
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.
Please try to avoid "utils" as a module name, as it's usually too generic to be a useful description. For instance, if this module is going to have a bunch of code related to TOML serialization, calling it toml.py would give a better indication of its purpose.
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.
Done
| Contstant for backup restore app | ||
| """ | ||
|
|
||
| TOML_PACKAGE_NAME = "package.toml" |
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.
For now, please just define this in the module that uses it, instead of making a new module for it.
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.
Done
openedx_learning/apps/authoring/backup_restore/management/commands/lp_dump.py
Outdated
Show resolved
Hide resolved
f4e70f8 to
9191ec9
Compare
Thanks for taking the time to review! I've handled all the comments |
|
(On my phone while traveling right now, so this is going to be terse, sorry.)
|
9191ec9 to
0cc1b64
Compare
ormsbee
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.
Two small notes for the future:
- Please don't squash your commits until the end of the review cycle. It makes it harder to see what things have changed.
- Your commit message should have a more details about why these changes were done.
I'll add the extra info in the commit message this time.

Description
Fixes: #331
These changes are the first step toward implementing a dump/load format for libraries. This initial approach introduces a minimal
lp_dumpmanagement command for Learning Core.The contents of
package.tomlare shown belowAcceptance Criteria
App requirements:
lp_dumpcommand in a newopenedx_learning.apps.authoring.backup_restoreapp.api.pymodule in that new app, which the management command would call.In terms of the output of the
lp_dumpcommand itself: