-
Notifications
You must be signed in to change notification settings - Fork 0
Add creator biographical information to EAD XML exports #5
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
base: main
Are you sure you want to change the base?
Conversation
- Add get_creator_bioghist method to fetch biographical notes - Update task_resource to resolve linked_agents - Inject bioghist element into EAD XML after </did> tag - Follow existing pattern for custom XML injection Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
- Use xml.sax.saxutils.escape for proper XML escaping - Format bioghist notes as separate paragraphs - Remove manual XML escaping in favor of library function Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
- Make error message more specific about the operation that failed Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
arcflow/main.py
Outdated
| 'ead3': 'false', | ||
| }) | ||
|
|
||
| # add record group and subgroup labels to EAD inside <archdesc level="collection"> |
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.
Make sure to update the comments to reflect the new behavior
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.
Updated in commit db640ce. The comment now reflects that the section handles multiple types of custom XML elements (record group/subgroup labels and biographical/historical notes).
alexdryden
left a comment
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.
Before I continue with the review, I'd like for you to start over re-work this with more frequent and verbose commits. So, keep everything the same, but parse out the changes more clearly through more frequent, single purpose commits.
Even if humans committing to this repo have dense commits, I'd like for you to be more explicit. For example, in the first commit to main.py you are making changes to the like locating the closing </did> tag, which is great, but not really part of "Add creator biographical information to EAD XML exports".
This makes it easier to review and helps me understand your specific logic and choices.
This reverts commit 63ffe60.
This reverts commit 3c608de.
This reverts commit e49106b.
Restructure the XML injection code to make it easier to add additional XML elements. Changes: - Move XML decode/encode outside the label-conditional block - Calculate insertion point (after </did> tag) upfront - Use extra_xml variable to accumulate multiple XML elements - Maintains existing recordgroup/subgroup functionality Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Include linked_agents in the resource API call resolution to fetch agent information along with the resource. This prepares for extracting creator biographical information. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Add import for xml_escape function to properly escape XML special characters (ampersands, less-than, greater-than, quotes) when injecting content into EAD XML. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Implement method to fetch biographical/historical notes from ArchivesSpace agent records. The method: - Filters agents by 'creator' role - Fetches agent details via API - Extracts note_bioghist content from subnotes - Escapes XML special characters using xml_escape - Wraps each note in <p> tags for proper XML structure - Returns concatenated paragraphs or None if no notes found Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Call get_creator_bioghist to fetch biographical notes and inject them into the EAD XML as a <bioghist> element. The bioghist element is added after </did> tag, following recordgroup/subgroup labels. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Update the comment on the XML injection section to indicate that it now handles multiple types of custom XML elements (record group/ subgroup labels AND biographical/historical notes), not just labels. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
I've reverted the previous commits and restructured the changes into 6 granular, single-purpose commits:
Each commit now has a clear, focused purpose that's easier to review. |
|
@copilot Great! This commit style suits me much better for this repo. Can you add a .github/copilot-instructions.md file with some basic on boarding instructions that includes this commit style? Consult https://docs.github.com/en/enterprise-cloud@latest/copilot/tutorials/coding-agent/get-the-best-results if you need context for what this file is for. Keep it very simple for now and mainly focus the commit style. Make the file so it is easy and clear for another agent to add more instructions later. |
Create onboarding instructions for GitHub Copilot agents with focus on granular, single-purpose commit style. Includes: - Guidelines for commit granularity - Examples of good vs bad commit sequences - Commit message format recommendations - Instructions for extending the file Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Created in commit b485f55. The file documents the granular commit style with clear examples and is structured to be easily extended by other agents. |
|
@leonelramirez I was in the middle of this right before break. Other than adding the note_bioghist field, this adds a few minor enhancements summarized above under the heading "Quality Improvements". This all looked good to me before break, but could you take a quick look to confirm? |
|
@copilot I'm working through a display issue in Arclight where raw xml is showing up in the display, and it looks like in addition to the Follow the standard approach used in ArchivesSpace and Arclight to prepare the |
I need some clarification before implementing: Questions:
Understanding these will help me implement the right solution with clean commits. |
|
@copilot Here is a little more detail:
Let me know if that makes sense and if you can see a solution on your own, but run your solution by me before you implement it. |
Yes, I understand the issue now. The problem is on line 549 in Current (incorrect): escaped_content = xml_escape(subnote['content'])
bioghist_paragraphs.append(f'<p>{escaped_content}</p>')This escapes ALL content, including legitimate EAD XML tags like Proposed solution: The bioghist content from ArchivesSpace already contains properly formatted EAD XML markup. We should pass it through as-is without escaping, since:
Change: # Remove xml_escape() for bioghist content - it's already valid EAD XML
bioghist_paragraphs.append(f'<p>{subnote["content"]}</p>')Key distinction:
Should I proceed with removing the |
|
@copilot Yes, that looks like the correct solution and your analysis is spot on. Proceed. |
The bioghist content from ArchivesSpace already contains valid EAD XML markup (e.g., <emph>, <title> tags) that should be preserved as actual XML nodes, not escaped text. Only recordgroup/subgroup labels need escaping since they're plain text strings. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Add guidelines for handling plain text vs structured XML content in the EAD pipeline. Explains when to use xml_escape() and when to pass content through unchanged, with rationale for the Traject and ArcLight display pipeline. Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Completed in commits fa57624 and 167cfe7:
The bioghist content now passes through unchanged, preserving tags like |
leonelramirez
left a comment
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.
Everything looks good to me. Nice job!
|
Need to add in the paragraphing for the bioghist, as an example, here is what we are pulling storing: Concerned by the lack of resources available to American choral directors, Robert Landers (director of the US Air Force Choir), Archie Jones (director of choirs at the University of Texas), and Maynard Klein (director of choirs at the University of Michigan) first conceived of a national choral association in 1957. The American Choral Directors Association (ACDA), originally called the American Choirmasters Association following the American Bandmasters Association, was founded two years later in Kansas City, Missouri during the biennial conference of the Music Teachers National Association. The ACDA, which began with 35 members, was established to serve the needs of pubilc school, collegiate, community, sacred, and professional choirs by sponsoring choral workshops, international conventions, concert exchanges, and symposia. |
alexdryden
left a comment
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.
We are going to need more structure to the bioghist content. Two things are happening:
- content is being read in with line breaks, and those need to be represented as xml paragraphs with individual
<p>tags for each paragraph. - in cases where there are multiple creators, there is no way to distinguish in bioghist which content belongs to which creator.
So, we need to make sure that we are capturing the formatting in the xml by using xml paragraphs.
We also need to devise a way to represent multiple creators' biographies in the bioghist element. EAD permits nesting of multiple bioghist elements. Order matters a lot for solr, so we'll need a way to be sure that the order of the creators in bioghist matches the order in the <origination>. If it isn't practical to verify that at this stage, just let me know and we'll keep thinking. In any case, put the name of the creator in a tag before the narrative content is inserted. This should be done even in cases where there is only one creator.
... The comment appears incomplete - could you provide the example of what we're pulling/storing? This will help me understand the structure better.
I understand you need:
Before implementing, I have questions: For paragraphing: Should I split content on For multiple creators: What structure do you prefer?
For creator names: Where should the name go? In a Could you complete the thought about where to put the creator name, and clarify the preferred structure? |
|
@copilot for the incomplete comment, it looks like a formatting issue with quoting xml syntax in markdown in the comment--regardless, that comment was a note to myself and you can ignore it. For your questions: I'm not an expert in EAD, so check these instructions against generally accepted practices and let me know if anything looks off. |
|
@copilot see my updated comment above--I needed to escape some of the xml tags |
Understood. I'll implement:
EAD Standards Check:
The approach looks compliant with EAD standards. I'll also ensure:
Proceeding with implementation. |
Implement improved bioghist structure: - Create separate <bioghist> element for each creator - Add <head> element with creator name (XML-escaped) - Include id attribute with ArchivesSpace agent ID - Split content on single newlines to create <p> paragraphs - Maintain linked_agents order for consistency with origination Co-authored-by: alexdryden <47127862+alexdryden@users.noreply.github.com>
Implementation Plan for Creator Biographical Information
Summary
Successfully restructured the implementation with granular, single-purpose commits:
Documentation Added
Created
.github/copilot-instructions.mdwith onboarding instructions for GitHub Copilot agents, documenting:XML Formatting
Bioghist Structure
Creates EAD-compliant nested
<bioghist>elements for each creator with:<bioghist>element, enabling clear distinction between multiple creatorsaspace_{agent_id}for unique identification<p>tags while preserving existing EAD XML markuplinked_agentsin order to maintain consistency with<origination>element order (important for Solr indexing)Example output structure:
Original prompt
This pull request was created from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.