-
Notifications
You must be signed in to change notification settings - Fork 2
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(cff): enforce valid urls as doi #108
Merged
Merged
Changes from all commits
Commits
Show all changes
15 commits
Select commit
Hold shift + click to select a range
dc49d66
fix(cff): parse yaml to handle quotes + prepend scheme to doi
cmdoret d0bd177
chore(fmt): quotes
cmdoret 6ba62c0
chore: update lock
cmdoret f125596
docs(readme): update obsolete env var name
cmdoret 2ee8edf
docs(readme): lighter fmt
cmdoret 4f40840
test(cff): update doctest to include scheme
cmdoret 43465c4
fix(cff): ensure doi is a valid url
cmdoret db81df7
fix: drop unused import
cmdoret ff6bd5d
fix(cff): error handling on invalid yaml
cmdoret 7d83123
refactor(cff): use regex + defensive prog
cmdoret fbae498
fix(cff): drop unused prefix var
cmdoret 159a65e
test(cff): update test cases with valid doi
cmdoret 4d43f42
feat(cff): add warning when doi missing from cff
cmdoret 94d2369
fix(log): replace deprecated logger.warn -> logger.warning
cmdoret 028cbd7
feat(log): format to include loglevel
cmdoret File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 is very readable - so I even dare to make a suggestion - though I'm not sure about the performance considerations: Instead of having 2 if statements taking care of multiple possible doi formats - would it not be simpler to regex the "10.xxxx" part of the doi out of whatever format is found in, and always prefix that with https://doi.org?
that way it can also handle any misspelling in the prefix in the cff (e.g. http://doi.org/)
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 point, that's a bit more robust :) I've added regex based matching.
The performance cost is negligible (in gimie time-scale):
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.
To avoid crashing the whole program because of malformatted user-entered data, I've also made it a bit more defensive:
Does that make sense?
In all cases above, the program resumes successfully. warnings are logged to stderr and will not be mixed with the output.
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 would only doubt a bit on the
It's not mandatory from CFF, but a doi is the only thing we are looking at a CFF for here - maybe we could just produce a warning in this case too? Basically, the only case in which a warning is not produced is either a user has not added a CFF file, or the CFF file they uploaded contains a valid DOI. But maybe that's being too conservative and patriarchal to our users - as if they are not aware that their CFF doesn't contain a DOI at all.
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 makes sense! I added a warning in 4d43f42
Now the behavior is:
EDIT: Added formatting to the log messages to include level (DEBUG/INFO/WARNING/ERROR)