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

fix(cff): doi structure parsing #121

Merged
merged 81 commits into from
Dec 17, 2024
Merged

fix(cff): doi structure parsing #121

merged 81 commits into from
Dec 17, 2024

Conversation

rmfranken
Copy link
Member

The DOI in a CFF is wrapped in an identifier object like so:

identifiers:
- type: doi
  value: 10.5281/zenodo.3555620

Our script was looking for doi objects like this:

doi: 10.5281/zenodo.3555620

Only the first case is a valid CFF format, so we must iterate through the identifier's, pick only the ones that are typed as doi, and return the value.

rmfranken and others added 30 commits November 14, 2024 11:09
Adds citation file (cff) to gimie repository for documenation and testing purposes.
…ure. Update pyshacl, comment out useless pyshacl test throwing errors
duplicate check
Co-authored-by: Cyril Matthey-Doret <cyril.matthey-doret@epfl.ch>
Co-authored-by: Cyril Matthey-Doret <cyril.matthey-doret@epfl.ch>
@rmfranken rmfranken requested a review from cmdoret December 17, 2024 08:10
@rmfranken
Copy link
Member Author

Only commit 4f5ef97 onwards is actually new/relevant. Soz

Copy link
Member

@cmdoret cmdoret left a comment

Choose a reason for hiding this comment

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

A few questions and comment to make sure that this is the desired behavior, but in principle it's good 👍

gimie/parsers/cff.py Outdated Show resolved Hide resolved
gimie/parsers/cff.py Outdated Show resolved Hide resolved
tests/test_cff.py Show resolved Hide resolved
@cmdoret cmdoret changed the title bug(CFF): Hotfix cff doi fix(CFF): Hotfix cff doi Dec 17, 2024
@cmdoret cmdoret changed the title fix(CFF): Hotfix cff doi fix(CFF): cff doi parsing Dec 17, 2024
@cmdoret cmdoret changed the title fix(CFF): cff doi parsing fix(cff): doi structure parsing Dec 17, 2024
@rmfranken rmfranken requested a review from cmdoret December 17, 2024 16:25
@rmfranken
Copy link
Member Author

Thanks for your good points!

Copy link
Member

@cmdoret cmdoret left a comment

Choose a reason for hiding this comment

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

I added a commit to reduce nesting, but besides that it looks good to merge. 🙌

But please squash commits when merging this PR

gimie/parsers/cff.py Outdated Show resolved Hide resolved
Co-authored-by: Cyril Matthey-Doret <cyril.matthey-doret@epfl.ch>
@rmfranken
Copy link
Member Author

Thank you for the nesting tip - looking at your change it does make sense. I was under the impression that the nesting didn't matter - if my first
try: identifiers = cff.get("identifiers", [])
would evaluate as false, that it would not continue the rest of the try block. If that's a bad assumption, I can see how splitting it into two blocks would speed it up.

also: return doi_urls or None is 🇳🇮 ce . That's neater in any case than if'ing it.

@rmfranken rmfranken merged commit 3867d7a into main Dec 17, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants