-
Notifications
You must be signed in to change notification settings - Fork 360
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
[MRG] Add Figshare content provider #788
Conversation
moving common functions to a DoiProvider
cb0a46e
to
e6cd4b1
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.
I left a few small comments, overall looks good to me!
Thanks for taking a look so quickly! I will investigate the versioning. |
@betatim |
@betatim I think this is ready now, can you take another look, please? |
Sure thing! We also change the title from WIP to MRG when it is ready for reviewing (and then maybe merge). MRG is a bit of a misnomer really it is more like "review please and then merge if you are happy" but yeah. |
@property | ||
def content_id(self): | ||
"""The Figshare article ID""" | ||
return "{}.v{}".format(self.article_id, self.article_version) |
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.
🎉
Merged! Could you make a PR that adds a line to the change log for this? |
Sure (my bad), #792 Also: jupyterhub/binderhub#951 |
[MRG] Add Figshare content provider
Also needs UI update: jupyterhub/binderhub#938