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 getLinkTarget for MEI 5 work titles #505

Merged
merged 14 commits into from
Jan 9, 2025

Conversation

bwbohl
Copy link
Member

@bwbohl bwbohl commented Dec 17, 2024

Description, Context and related Issue

Remove mei:titleStmt when determining the title of the work because in MEI 5 mei:title is a direct child of mei:work
Closes #337

How Has This Been Tested?

With a BAZ-GA MEI 5 work.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)

Overview

  • I have performed a self-review of my code, according to the style guide

  • I have read the CONTRIBUTING document.

  • I have added tests to cover my changes at testing

  • All new and existing tests passed.
    --> will know when the PR has run its tests ;-)

@bwbohl bwbohl added the Type: bugfix A pull request providing a bugfix label Dec 17, 2024
@bwbohl bwbohl added this to the 1.0.0 milestone Dec 17, 2024
@krHERO krHERO requested a review from peterstadler December 17, 2024 12:25
@peterstadler
Copy link
Member

Only from looking at it (i.e. without testing) that looks ok. Yet I wonder whether the whole conditional is overly complicated and something like //work//title[1] for every case would be enough?
Otherwise it would be very helpful to have some inline documentation that records the motivation for the different cases.

@krHERO
Copy link
Member

krHERO commented Dec 18, 2024

at the community meeting 2024-12-18 we agreed to add inline documentation regarding MEI versions for the v1.0.0 release @bwbohl and to have a more complex solution in a later release

@bwbohl bwbohl force-pushed the fix/getLinkTarget-mei5-work branch 2 times, most recently from 816a3d9 to c807e7a Compare December 19, 2024 08:28
@bwbohl bwbohl requested review from riedde and peterstadler and removed request for peterstadler December 19, 2024 08:57
@bwbohl
Copy link
Member Author

bwbohl commented Dec 19, 2024

Ok, I tried to simplify and globalise a bit in the local:getWindowTitle. For a later release we definitely have to revisit all object assumption and put those in corresponding XQuery modules, e.g. work.xqm also was a work:isWork function but the assumptions there are not identical to those in getLinkTarget.xql, that’s why I did not use it for now.

@riedde please test with your dataset that has mei:work as root elements in your work encodings. I heavily reduced that bit of the function because $doc//mei:work is true for all the cases we had before. I introduced a worhTitleContainer variable to deal with the MEI versions.

@bwbohl bwbohl force-pushed the fix/getLinkTarget-mei5-work branch 2 times, most recently from 1999684 to 65d1201 Compare December 19, 2024 09:09
@roewenstrunk roewenstrunk self-requested a review January 7, 2025 14:37
@peterstadler peterstadler force-pushed the fix/getLinkTarget-mei5-work branch from 531900a to babd1c6 Compare January 7, 2025 17:02
add/data/xql/getLinkTarget.xql Outdated Show resolved Hide resolved
Copy link
Member

@roewenstrunk roewenstrunk left a comment

Choose a reason for hiding this comment

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

Rebased locally and tested with Klarinettenquintett and Pintos

peterstadler and others added 2 commits January 9, 2025 11:53
since the return value of `eutil:getDoc#1` in https://github.com/Edirom/Edirom-Online/blob/babd1c6076ba58214b85a9891c3c2215d9d1c9d6/add/data/xql/getLinkTarget.xql#L278 might be the empty sequence this should be reflected in the function signatures that consume this $doc variable.
@peterstadler peterstadler force-pushed the fix/getLinkTarget-mei5-work branch from 8f7738d to d065dfd Compare January 9, 2025 10:53
@peterstadler peterstadler self-requested a review January 9, 2025 10:57
@peterstadler peterstadler merged commit 6ce8986 into develop Jan 9, 2025
5 checks passed
@peterstadler peterstadler deleted the fix/getLinkTarget-mei5-work branch January 9, 2025 10:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: bugfix A pull request providing a bugfix
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

getLinkTarget.xql fails if window title not found for mei:work
4 participants