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

Updated keyword linking script to also update the URI of existing links #482

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hakonhagland
Copy link
Collaborator

Builds on #478, which should be merged first.

Updated keyword linking script to also update the URI of existing links if needed.

This was referenced Jan 13, 2025
@blattms blattms requested a review from lisajulia January 14, 2025 12:25
@blattms
Copy link
Member

blattms commented Jan 14, 2025

THis is quite obfuscated as includes the other PR.

Please explain what the changes to the script are.

I guess the problem of broken links arises whenever someone uses libreoffice to change a keyword file

@hakonhagland
Copy link
Collaborator Author

Please explain what the changes to the script are.

@blattms Sorry, the changes to the script are in the last commit, and can be seen here: ef26576, the other commits belong to #478

@hakonhagland
Copy link
Collaborator Author

I guess the problem of broken links arises whenever someone uses libreoffice to change a keyword file

@blattms Yes maybe... another thing is that many of the links may not be broken, but linking to another version of the same keyword. For example, the ADD keyword exists in multiple chapters as noted in #478 (comment). The changes in to kw_uri_map.txt in #481 imposes a fixed order on those keywords, if they did not adhere to that order before, they will now need to be relinked.

@lisajulia
Copy link
Collaborator

@hakonhagland: can you include the commit from #481 here possibly? Then this check https://github.com/OPM/opm-reference-manual/actions/runs/12757567531/job/35558097602?pr=482 should pass, right?

@hakonhagland
Copy link
Collaborator Author

can you include the commit from #481 here possibly? Then this check ... should pass

@lisajulia Yes, I have now included that commit. But now the second check fails. To make this check pass we need to update the URIs for most keywords as noted in #482 (comment). I have started on this process in #483. However, I think this failing check should not prevent this PR from being merged as the purpose of this check will be as an aid for future PRs.

@lisajulia
Copy link
Collaborator

ok, but then thick check that is failing now will only succeed once all neccessary URIs are updated and merged to master, i.e., when #483 is ready and merged, right?

@hakonhagland
Copy link
Collaborator Author

will only succeed once all neccessary URIs are updated and merged to master, i.e., when #483 is ready and merged, right?

Yes, but I guess it will not succeed even after #483 is merged, since that is only for chapter 4, we need to also update links in chapter 5, 6, 7, ... and so on. Which will be done in separate PRs after #483 has been merged

@lisajulia
Copy link
Collaborator

lisajulia commented Jan 17, 2025

will only succeed once all neccessary URIs are updated and merged to master, i.e., when #483 is ready and merged, right?

Yes, but I guess it will not succeed even after #483 is merged, since that is only for chapter 4, we need to also update links in chapter 5, 6, 7, ... and so on. Which will be done in separate PRs after #483 has been merged

Ok, how about moving the addition of this file, i.e. the introduction of this new test, to a separate commit and merge this when it's green? Sorry about being so picky, but I'm not a fan of merging things that make the pipeline red 🤔

@hakonhagland
Copy link
Collaborator Author

I'm not a fan of merging things that make the pipeline red

@lisajulia I have deactivated the second check, so all checks should pass now.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
Updated keyword linking script to also update URI of existings links if
needed.
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