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

adjust urn regex #2035

Closed
wants to merge 7 commits into from
Closed

adjust urn regex #2035

wants to merge 7 commits into from

Conversation

maipet
Copy link
Contributor

@maipet maipet commented Jul 24, 2024

@maipet maipet requested a review from fsteeg July 24, 2024 08:28
Copy link
Member

@fsteeg fsteeg left a comment

Choose a reason for hiding this comment

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

Updated the web app tests for the new test data to fix the build.

Looking at the code I'm wondering if the match-regex shouldn't be the same as the replace-regex, to make sure it is actually replaced if it matches? But will assign @TobiasNx for proper functional review.

@fsteeg fsteeg requested a review from TobiasNx July 25, 2024 08:10
@TobiasNx
Copy link
Contributor

Functionally this looks good. Next time combine the changes in the Fix and the changes in the testfiles in one commit.

Otherwise is this hint from @fsteeg a must? Is there a scneario where there is a urn without http?

Looking at the code I'm wondering if the match-regex shouldn't be the same as the replace-regex, to make sure it is actually replaced if it matches?

Otherwise +1

@TobiasNx TobiasNx assigned maipet and unassigned TobiasNx Jul 29, 2024
@maipet maipet assigned TobiasNx and unassigned maipet Jul 29, 2024
@TobiasNx
Copy link
Contributor

TobiasNx commented Jul 30, 2024

We talked about it off board, lets try to create the urn and the resolver links differently:

Fetch all urn from the links AND copy all urn links that are marked Resolver-System in $x without formatting except doi links.

PS: Also we should add an URN Link if no URN Resolver System Link is added but an URN exists in 024 (added Aug 5th 24)

@TobiasNx TobiasNx assigned maipet and unassigned TobiasNx Aug 5, 2024
@TobiasNx
Copy link
Contributor

TobiasNx commented Aug 6, 2024

@maipet I added some commits to better handle the URNs and Resolver Links. The links in 865 with Resolver System were already used but were skipped if they had a nbn refrence in them. I specified the regex pattern a little bit more and decoupled the URN links in 865 from the URN in 024 and I only use the URN from 024 as fallback for links.

Additionally I have changed the URL for nbn links to org instead of de since that seems to be the standard.

Could your review it? And also should we create a new PR with less commits?

@TobiasNx
Copy link
Contributor

TobiasNx commented Aug 7, 2024

This pr is replaced by #2043

@TobiasNx TobiasNx closed this Aug 7, 2024
@TobiasNx TobiasNx mentioned this pull request Aug 12, 2024
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