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 files so that they are validated by the schema #15

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

CThierrin
Copy link
Contributor

@CThierrin CThierrin commented Jun 14, 2024

Resolves #8, resolves #12, and resolves #13, and adds comments to scripts

@CThierrin CThierrin requested a review from dchiller June 14, 2024 17:59
@dchiller
Copy link
Contributor

I'm confused by the diff of this PR. Why are 2000 + files changed to fix #8?

@CThierrin
Copy link
Contributor Author

I'm confused by the diff of this PR. Why are 2000 + files changed to fix #8?

#8 was hiding other issues with the files, each file was reprocessed through the newest version of liberupdatev5

@dchiller
Copy link
Contributor

#8 was hiding other issues with the files

It would be a good idea to put whatever issues these were into an issue and/or in the description of this PR: what would another developer need to know about what changes were made with this PR and why?

liberbatch.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/mei-all.rng Outdated Show resolved Hide resolved
Liber Usualis - mei3/meichecker.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/meichecker.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/liberbatch.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
Comment on lines 40 to 42
for att in list(child.attrib):
if att.endswith("href"):
child.attrib['target'] = child.attrib.pop(att)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems counterintuitive to me to iterate through the child attributes to get a single attribute?

Why not child.get("href") (maybe you have to namespace it)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

iteration is used to avoid problems with namespace

Copy link
Contributor

Choose a reason for hiding this comment

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

Are the namespaces of the liber MEI 3 files inconsistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sometimes the namespace shows up, sometimes it doesn't

Copy link
Contributor

Choose a reason for hiding this comment

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

Like, different mei3 files will have different namespaces?

Do you have an example?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just ran a script to print out every unique attribute of the files. One attribute has {http://www.w3.org/1999/xlink} as a namespace, one has {http://www.w3.org/XML/1998/namespace}, the remaining have no namespace. I can look into removing the iteration for attributes with no namespace

Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
label = ""
for attr, value in child.attrib.items():
if attr.endswith("ulx") and int(value) < 0:
child.attrib["label"] = label + "ulx = " + value + " "
Copy link
Contributor

Choose a reason for hiding this comment

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

So on zone elements, you are setting the label attribute to "ulx = some_number " ? The label attribute looks like is designed for a tooltip or some such?

Why not use the Element.set method here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Code rewritten to use child.set

The label attribute here is used when ulx or lrx is negative to save their original values

Copy link
Contributor

Choose a reason for hiding this comment

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

Is that necessary? We are saving the original files if we ever need those values and it seems to me we should be trying to have the MEI v5 files created be "up to code".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

label attribute removed

Liber Usualis - mei3/liberupdatev5.py Outdated Show resolved Hide resolved
@dchiller
Copy link
Contributor

Could you link the relevant issues to this PR?

Copy link
Contributor

@dchiller dchiller left a comment

Choose a reason for hiding this comment

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

There look like many places where this file has not been PEP 8 formatted (or for the lab's purposes, formatted by black). Let me know if you need help troubleshooting.

Comment on lines 50 to 54
for child_2 in root.iter():
for attr, value in child_2.attrib.items():
if attr.endswith("id") and value == pageref:
child.set("n", child_2.attrib["n"])
break
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be a find call, rather than iterating through the whole tree again:

root.find(".//meins:page[@xmlns:id='pageref']", namespaces = ...)

child.tag = "TODELETE"

# Handle system breaks
elif child.tag.endswith("sb"):
Copy link
Contributor

Choose a reason for hiding this comment

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

Same questions in this block as in the block about "pb"

label = ""
for attr, value in child.attrib.items():
if attr.endswith("ulx") and int(value) < 0:
label = f"{label} ulx = {value} "
Copy link
Contributor

Choose a reason for hiding this comment

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

Your label attribute will now always have a leading and trailing space if not empty...this doesn't seem right.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Label has been removed

Also label removed from zone
Also simplify attribute0-handling code for most attributes
- Neume components no longer just puntum
- Fix bug that removed content from elements
- new liberbatch_parr.py script that takes advantage of parallel processing
- MEI files output with proper indentation
Also fix octave glitch that arises when notationtype is set to neume
Scripts were used to update already-generated mei 5 files, now incorporated into larger liberupdatev5
@dchiller
Copy link
Contributor

dchiller commented Aug 5, 2024

@CThierrin It's very hard to review given the number of files in the PR (my browser gets really slow trying to render the diffs). Could you make a PR that just has the new MEI files and then have this PR be the changes to the code files... I don't think the MEI files themselves need my review, and then I'll be able to review the code here.

@CThierrin CThierrin mentioned this pull request Aug 6, 2024
@CThierrin
Copy link
Contributor Author

@dchiller The new MEI files can just get deleted, they need to be remade again

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants