Skip to content

Do not raise KeyError when dumping after a Format line redefinition - #38

Open
eeshsaxena wants to merge 1 commit into
chireiden:masterfrom
eeshsaxena:dump-format-redefinition
Open

Do not raise KeyError when dumping after a Format line redefinition#38
eeshsaxena wants to merge 1 commit into
chireiden:masterfrom
eeshsaxena:dump-format-redefinition

Conversation

@eeshsaxena

Copy link
Copy Markdown

Document.dump_file raises KeyError on a document that parse_string accepted, when a section has a second Format: line:

import io, ass
doc = ass.parse_string(
    "[Events]\n"
    "Format: Layer, Start, End, Style, Text\n"
    "Dialogue: 0,0:00:01.00,0:00:04.00,Default,Hello\n"
    "Format: a\n"
)
doc.dump_file(io.StringIO())   # KeyError: 'a'

The last Format: line wins for the section's field order, but the dialogue line was parsed under the earlier order and doesn't carry the new field names, so line.dump looks up a field the line doesn't have. Since parsing already succeeded, dumping shouldn't blow up. I made a missing field dump as empty, so a parsed document always round-trips. Added a test; the suite passes.

A section may contain more than one Format line, and the last one wins
for the section field order. Lines parsed under an earlier order do not
carry the new field names, so dumping them looked up a missing field and
raised KeyError even though parse_string had accepted the document.

Fall back to an empty value for a field a line does not carry, so a
parsed document always dumps.
@FichteFoll

Copy link
Copy Markdown
Member

Ensuring a roundtrip works is good, though I am a little unsure about whether we should rather error on load here or alternatively not write such a bogus format line and silently skip it.

Where/How did you encounter this file? Did you test how renderers or other ASS tools handle this case? My assumption would be that they ignore the second format line but you can never be sure in ASS world.

@eeshsaxena

Copy link
Copy Markdown
Author

Hi! Gentle nudge on this one whenever you have some bandwidth. It's a small, self-contained fix (Do not raise KeyError when dumping after a Format line redefinition), and it's currently mergeable with no conflicts. No urgency at all, and I'm happy to make any changes you'd like. Thanks for maintaining python-ass!

@FichteFoll

Copy link
Copy Markdown
Member

This fix is small code-wise but it can have non-trivial consequences that I have not had found the time to look into.

I'd also appreciate if I could still communicate with a human here that actually reads my comments instead of bumping their "small fix" after only 2 days.

@eeshsaxena

Copy link
Copy Markdown
Author

You're right, bumping after two days was too impatient of me and I'm sorry. There's a real person on this end. If it helps, I'm glad to write out the edge cases I think a Format redefinition can hit so you have something concrete to check against, no rush at all.

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