Skip to content
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

Validate pre and post convert hooks #1438

Merged
merged 2 commits into from
May 5, 2021
Merged

Conversation

tmetzl
Copy link
Contributor

@tmetzl tmetzl commented May 4, 2021

Fixing the problem mentioned in #1376.

If the hooks are given as an import string (see example) they are now properly imported.

nbgrader generate_feedback <assignment-id> --GenerateFeedback.post_convert_hook="my_package.my_hook"

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

@validate('post_convert_hook')
def _validate_post_convert_hook(self, proposal):
value = proposal['value']
if isinstance(value, string_types):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nbgrader is python 3 only these days, so I think you can just check against str here (and above as well).

@@ -19,6 +19,8 @@
from ..nbgraderformat import SchemaTooOldError, SchemaTooNewError
import typing
from nbconvert.exporters.exporter import ResourcesDict
from ipython_genutils.importstring import import_item
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe genutils isn't officially supported, and it would probably be better to not need to add a new dependency in any case---could you achieve the same behavior using importlib.import_module? See https://docs.python.org/3/library/importlib.html

@jhamrick jhamrick added this to the 0.7.0 milestone May 5, 2021
@tmetzl
Copy link
Contributor Author

tmetzl commented May 5, 2021

Thanks for pointing out the changes. I think I addressed them with my new commit.

Copy link
Member

@jhamrick jhamrick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, thanks!

@jhamrick jhamrick merged commit cc40da0 into jupyter:master May 5, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants