feat: bump RAS format to 1.3 #247
Conversation
|
Review changes with SemanticDiff. Analyzed 6 of 10 files. Overall, the semantic diff is 47% smaller than the GitHub diff.
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #247 +/- ##
==========================================
- Coverage 87.51% 87.12% -0.40%
==========================================
Files 21 21
Lines 1786 1786
Branches 323 323
==========================================
- Hits 1563 1556 -7
- Misses 185 191 +6
- Partials 38 39 +1 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Nice work, but requires a few changes.
For CI to pass on Windows (see https://github.com/ReadAlongs/Studio/actions/runs/11276460698/job/31360285739?pr=247) you also need to update test/data/cs-ref.readalong. I should change that test so it only looks at the line where not outputting utf-8 correctly would make a difference, instead of diffing the whole file...
More comments inline.
| dur CDATA #IMPLIED | ||
| annotation-id CDATA #IMPLIED | ||
| sentence-id CDATA #IMPLIED | ||
| xmlns CDATA #IMPLIED> |
There was a problem hiding this comment.
I'm not sure why we need xmlns here. I get it for the top-level read-along element, and I would get it if every element needed to allow it, but only one additional element I don't get.
| id CDATA #IMPLIED | ||
| class CDATA #IMPLIED | ||
| do-not-align CDATA #IMPLIED | ||
| ARPABET CDATA #IMPLIED |
There was a problem hiding this comment.
can we make this case insensitive?
There was a problem hiding this comment.
I was thinking the same. However, I am worried about backwards compatibility. Attributes in HTML are case insensitive but are case sensitive in XML (.readalong is a subset of XML). We need a good look at the whole pipeline before we make the switch. We use both XML and HTML parsers in the various parts of the pileline. As far as I know, only the CLIs consume the ARPABET attribute.
There was a problem hiding this comment.
Maybe an option is to put both ARPABET and arpabet is the dtd?
| id CDATA #IMPLIED | ||
| class CDATA #IMPLIED | ||
| do-not-align CDATA #IMPLIED | ||
| ARPABET CDATA #IMPLIED |
| @@ -20,13 +20,17 @@ | |||
|
|
|||
| from lxml import etree | |||
|
|
|||
| from readalongs._version import VERSION | |||
| from readalongs._version import CURRENT_WEB_APP_VERSION, VERSION | |||
There was a problem hiding this comment.
nice idea, moving CURRENT_WEB_APP_VERSION to _version.py and reusing it here.
PR Goal?
To fix an issue created by the editor
Fixes?
ReadAlongs/Studio-Web#347
Feedback sought?
sanity check
Priority?
High
Tests added?
yes
How to test?
Confidence?
High
Version change?
no