-
Notifications
You must be signed in to change notification settings - Fork 19
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: parse and clean archive badges and markdown links to URL #243
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #243 +/- ##
==========================================
+ Coverage 74.06% 75.25% +1.18%
==========================================
Files 10 10
Lines 671 699 +28
Branches 82 89 +7
==========================================
+ Hits 497 526 +29
+ Misses 166 162 -4
- Partials 8 11 +3 ☔ View full report in Codecov by Sentry. |
hey @banesullivan i'll leave some more specific feedback here but it looks like
|
I'm also noticing some inconsistency in JOSS: for joss / sleplet, there is a link to the paper. For nbcompare, there is a link to the DOI, which resolves to the paper! Both will work! My question is, should we be consistent in how we save the doi and always link to the doi rather than the paper in terms of the data we store in our "database," aka YML file? the archive value is inconsistent, too, but I think it's OK as is. Especially because sometimes it's a GitHub link and others it's Zenodo, and sometimes the Zenodo link is to the "latest" rather than the actual archive VERSION that we approved. So let's not worry about archive and focus on JOSS DOI as it hopefully isn't a huge amount of work. if it is, we are ok as is and we can open an issue about a future iteration that cleans it up just a bit more. So, the takeaways here are:
Thank you so much. This looks really great!! 🚀 If you'd like, we could merge this as is today, and you could work on the comments above in another pr. that would allow us to update the website tomorrow. if you'd like to complete all of the work in this pr, say the word, and we can hold off until January to get things merged. |
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.
oops. it looks like i left blocks of feedbak but not the line by line comments. i'm just making sure those are visible now.
@@ -7,6 +7,8 @@ | |||
from datetime import datetime | |||
from typing import Any | |||
|
|||
import doi |
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.
Since we've added a new dep, we should make sure that it is noted in the changelog and also document why we added it.
@@ -4,6 +4,8 @@ | |||
|
|||
## [Unreleased] | |||
|
|||
* Fix: Parse archive and JOSS links to handle markdown links and validate DOI links are valid (@banesullivan) | |||
|
|||
[v1.4] - 2024-11-22 |
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.
[v1.4] - 2024-11-22 | |
## [v1.4] - 2024-11-22 |
let's fix my mistake in the changelog :)
@@ -4,6 +4,8 @@ | |||
|
|||
## [Unreleased] | |||
|
|||
* Fix: Parse archive and JOSS links to handle markdown links and validate DOI links are valid (@banesullivan) |
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.
* Fix: Parse archive and JOSS links to handle markdown links and validate DOI links are valid (@banesullivan) | |
* Fix: Parse archive and JOSS links to handle markdown links and validate DOI links are valid. Added python-doi as a dependency (@banesullivan) |
i don't know how you handle a add and fix in one line but i think it's good to note that change here.
This utility will attempt to parse the DOI link from the various formats | ||
that are commonly present in review metadata. This utility will handle: | ||
|
||
* Markdown links in the format `[label](URL)`, e.g., `[my archive](https://doi.org/10.1234/zenodo.12345678)` |
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.
it looks like the issue that failed when i ran it locally using a zenodo badge
pyOpenSci/software-submission#83
honestly, I may have updated and added that (it is possible). But it would be good to parse a markdown badge url too.
let me know if you don't see that error but i saw it running locally.
archive = archive.replace("http://", "https://") | ||
# Validate that the URL resolves | ||
if not check_url(archive): | ||
raise ValueError(f"Invalid archive URL: {archive}") |
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 think we want to use pytest.raises
ValueError // match= to hit these missing lines in our coverage.
hey @banesullivan i'm checking in on this pr. we are running into the issue that every pr on the website fails CI because of the dois! i think we have two options.
The above approach is great if you are busy during this first full week back!! Please let me know what you prefer! |
This will parse markdown links/badges to consistently capture a URLs from the archive and JOSS DOI fields.
Note that this adds https://github.com/papis/python-doi as a dependency
The solution I landed on here is to always coerce the link to a single URL since this is what the review templates expect here:
https://github.com/pyOpenSci/pyopensci.github.io/blob/d0b561cc493f6e4691f171b4460b0f7d4793267a/_includes/package-grid.html#L42
Since this now validates the DOI links, I needed to use a real/valid DOI for the test data so I used PyVista's DOI for these 😄
I also noticed that some review issues had the JOSS archive key as
JOSS DOI
and others have it asJOSS
. The changes here account for that and ensure data are normalized toJOSS