Skip to content

getConcordances includes XML:IDs of concordances, groups and connections or generates a stable ID for them - #210

Open
DennisFriedl wants to merge 8 commits into
Edirom:developfrom
korngold-werkausgabe:ftr/21-ids-of-connections
Open

getConcordances includes XML:IDs of concordances, groups and connections or generates a stable ID for them#210
DennisFriedl wants to merge 8 commits into
Edirom:developfrom
korngold-werkausgabe:ftr/21-ids-of-connections

Conversation

@DennisFriedl

@DennisFriedl DennisFriedl commented Aug 12, 2026

Copy link
Copy Markdown
Member

Description, Context and related Issue

Refs #209

Includes the xml:id of connection elements in the objects. This makes it possible to navigate to a certain connection via their ID, see Edirom/Edirom-Online-Frontend#268

Now returns something like

{
   "id" : "connection2",
    "plist" : "....",
    "name" : "2"
  }, {
    "id" : "connection3",
     "plist" : "....",
     "name" : "3"
}

How Has This Been Tested?

Tested with Edition Example and internal Korngold data.

Types of changes

  • New feature (non-breaking change which adds functionality). The endpoint should work as normal as no existing values of the objects are modified.

@DennisFriedl

Copy link
Copy Markdown
Member Author

I modfied this PR: getConcordances will now use the function generate-id to give each concordance, group and connection a unique ID if it does not have a xml:id. This Id is the same on every request and allows the navigation to e.g. a connection (see Edirom/Edirom-Online-Frontend#269) even if this connection was not explicitly made identifiable with an xml:id by the editor. However, present xml:ids are respected and will not be overwritten.

@DennisFriedl DennisFriedl changed the title getConcordances also includes xml:ids of connections if present getConcordances includes XML:IDs of concordances, groups and connections or generates a stable ID for them Aug 14, 2026

@peterstadler peterstadler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The code looks really good and I agree that this can be merged without breaking backwards compatibility.

Yet the test are failing (as expected) so you should update the test files (the expected results) within this PR.

@peterstadler peterstadler left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good, and tests are successful 👍

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.

2 participants