-
Notifications
You must be signed in to change notification settings - Fork 11
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
Fix: Make the library more robust for publishing #41
Conversation
Codecov Report
@@ Coverage Diff @@
## master #41 +/- ##
==========================================
+ Coverage 94.51% 95.72% +1.20%
==========================================
Files 11 11
Lines 857 1005 +148
==========================================
+ Hits 810 962 +152
+ Misses 47 43 -4
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
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.
some minor comments from cursory review
return datacite_dict | ||
|
||
|
||
def _get_datacite_schema(): |
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 know that it is just a code move.
But this better be cached or (even without any analysis of how often this is called) we might end up with all kinds fails upon some intermittent connection issues etc.
Or might be better to just include that file right here in the dandischema sources/distribution -- who knows what RFing they would decide to do in datacite etc. Then this file would always be with us and reliably available -- would make it more robust.
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.
that's why i have left that by default as False
. we should have some trust no?
people can retry on http errors for anyone using the code. i will at least make it a permalink.
if "Published" in cls.__name__: | ||
tempval = "Published" + tempval | ||
if "BareAsset" == cls.__name__: | ||
tempval = "Bare" + tempval |
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.
so it could make it BarePublished<val>
? shouldn't Published
be added later so it would be PublishedBare<val>
?
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.
that should not happen here. the second if is an equality check, in which case the only value tempval should have is Asset
, thus turning it into BareAsset
. anything else it should throw an error in the next few lines.
Co-authored-by: Yaroslav Halchenko <[email protected]>
@yarikoptic - feel free to merge this tomorrow if you have no other changes to make. |
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 we talk about "more robust": I still think we should cache online fetching, and if someone is to retry -- it is for a fetcher . But that could be done later, so I am ok to proceed
closes dandi/dandi-archive#347