Repository navigation
Conversation
…eate
point(), segment(), range_() and page() port TiLiA's validate_creation for
point-like and segment-like components, the range timeline's
_validate_component_creation, and the PDF importer's page check, with
TiLiA's reasons word for word. point() takes the component's kind, which
names it in the reason ("PDF page marker"), and formats times as TiLiA's
format_media_time does.
Part of TimeLineAnnotator/project-management#265
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The rules are about to grow into the one set that everything creating or changing a component goes through: the importers, and later bulk edits and the reader and writer. So they leave the importers package. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
tilia_core.rules now holds every rule for a valid marker, hierarchy, range,
harmony, mode, PDF marker and beat: one function per kind, which takes the
media length, the timeline's other components and the attributes to check,
and gives (True, "") or (False, reason) for the first rule broken. Where
TiLiA's checks at creation, in set_data and in its operations disagree, the
strictest applies, so some rows TiLiA's importers accept are refused:
- a time of nan, with the text the kind's importer gives a time that isn't
a number (not_valid, which the importers can use for that text too);
- a label, comments or custom text that isn't text, and a color Qt can't
read, checked without Qt against the forms and names QColor accepts;
- a hierarchy that starts before 0, has a level below 1, a pre_start
outside 0 to its start or a post_end outside its end to the media
length, or crosses another (TiLiA's get_boundary_conflicts);
- a range whose pre_start is after its start or whose post_end is before
its end, or whose joined_right isn't the next range in its row;
- harmony and mode values TiLiA refuses in set_data: steps, accidentals,
levels, display modes, font types, and qualities, inversions and mode
types read from the installed music21, imported only when a harmony or
a mode is checked. Without music21, the other rules are checked, and
CantCheck names the command that installs the harmony extra.
A negative time is now shown with a minus sign ("-00:01.0") rather than
wrapped around an hour. A broken rule between components carries the ids
of the others involved (Outcome.others). range_rows checks a range
timeline's rows.
Part of TimeLineAnnotator/project-management#291
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Every reason for a value that isn't one, whatever the kind, now reads "<value> is not a valid <attribute>.": a time of nan, a level or page number that isn't an integer, a text that isn't text, a color, a harmony value, a row's id and name. not_valid(attribute, value) gives it, for the importers to give the same for a value they can't read. Two rows with the same id give "There is already a row with the id Ab12Cd.". Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
azfoo
force-pushed
the
v1/291-component-rules
branch
from
October 14, 2026 12:20
a955dec to
9f46077
Compare
azfoo
marked this pull request as ready for review
October 14, 2026 12:21
`if rules.marker(...)` was always true, since an Outcome is a non-empty tuple. It now reads as "keeps to the rules": bool(outcome) is outcome.ok, and it still unpacks as (ok, reason). The rules were chained with `or`, which relied on a broken outcome being true; they are now checked in order by _first, each only once the ones before it are kept. The nesting test now covers levels from 1 to 8. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The 147 colour names were written out in rules.py: a copy of CSS 3's
(SVG 1.0's), which is what QColor.colorNames() gives, with
"transparent". They now come from webcolors.names("css3"), and the core
adds only "transparent". webcolors (pure Python, BSD-3-Clause, no
dependencies; >=24.8.0, the first with names()) becomes tilia-core's
second dependency; music21, and so the app, already install it. A test
checks that every CSS 3 name and "transparent" are colours, and CSS 4's
"rebeccapurple", which Qt doesn't read, isn't.
Part of TimeLineAnnotator/project-management#291
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ROW_ID_LENGTH and ROW_ID_CHARACTERS give the six letters and digits of a range row's id, as TiLiA's generate_row_id makes them, so that the importers make ids with the same format range_rows accepts. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Outcome is a tuple subclass whose __new__ takes the reason and the others apart, so copy, deepcopy and pickle rebuilt it from the pair alone, and CantCheck's pickled message came back as its kind. __getnewargs__ and __reduce__ give them what they need. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_is_number accepted any int, and the time checks then raised OverflowError when they converted it. It is now refused with the text for a value that isn't one. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_cross ignored a hierarchy whose level wasn't an int, so one with level 1.0 couldn't be crossed, although _same_place compares 1.0 with 1. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A time just under a minute showed 60 seconds: 59.96 s was 00:60.0, as TiLiA's format_media_time shows it. It is rounded first, so it is 01:00.0 (TimeLineAnnotator/project-management#304 fixes TiLiA). Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
An importer keeps a row whose color isn't one, with the default color. A bad color was checked with the rules for every kind, before the others, so it hid any other broken rule of the same component, and the row was kept. The color is now checked last, for every kind and for a range timeline's rows. The test that TiLiA creates a component with any color goes: TiLiA's creation rules don't take a color. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The stand-ins for TiLiA's creation rules ignore what they don't take, so asserting that they accept a harmony's or a mode's values tested nothing. The comment above them now says which values TiLiA only checks in set_data. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
new_row_id took its length and characters from a copy of its own, so the ids it made and the ids range_rows accepts could drift apart. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Checking a harmony or a mode with music21 installed but failing to import, for example against a numpy it can't use, raised that ImportError. It now raises CantCheck, as without the extra, with the ImportError as its cause. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
CantCheck tells people to run pip install "tilia-core[harmony]", which installed nothing while the extra didn't exist. The extra brings music21, which only the harmony and mode checks, and later the harmony importer, import. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Outcome.__bool__ returned ok as it was given, so bool() raised TypeError for an outcome made with 1 or a numpy bool. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
_cross took a level as a number only if a float could hold it, so a hierarchy with a level beyond that couldn't be crossed, although the level rule accepts any integer. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Without music21, a harmony or a key whose color isn't one gave the color's reason, not CantCheck, so an importer, which keeps a row with the default color, kept it with its quality or type unchecked. The color was checked inside the chain, before the decision on music21. One helper, _kept, now runs a kind's rules in order, then raises CantCheck when music21 is missing, then checks the color, so every kind gets that order from one place. A step that gives a kept outcome no longer ends the checks. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only ImportError was caught, so a music21 that raised something else on import, such as an AttributeError from a numpy it can't use, still crashed the check. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
It told people to install the harmony extra even when it was installed and music21 failed to import, so a printed report gave no way to a fix. It now says that music21 won't import, and why. The text for a missing extra is unchanged. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Python keeps no module whose import failed, so each harmony or key checked tried the failing import again. The failure is kept for the session. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
azfoo
force-pushed
the
v1/291-component-rules
branch
from
October 19, 2026 09:14
5709155 to
735d494
Compare
This reverts 735d494. Keeping the failure for good meant that installing a missing dependency in a notebook didn't help until a restart, kept the failed import's frames alive, and made the tests depend on their order; and it left music21's absence looked up every time anyway. The loops it was for go through check_timeline, which can look music21 up once per call (TimeLineAnnotator/project-management#303). Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
When music21 was found but wouldn't import, the message said it was installed and gave no install command. But a leftover music21 folder, or a missing dependency, also gives an ImportError, and installing the extra fixes both. The message now gives the reason and the command, and doesn't claim music21 is installed. The text for a missing extra is unchanged. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Since CantCheck carried the import error, copying or pickling one whose error class needs arguments to be built raised TypeError. The error isn't carried any more: its text is in the message, which is. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Only __bool__ converted it, so .ok and unpacking still gave 1, or a numpy bool, when the outcome was made with one. It is converted once, when the outcome is made. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
A hierarchy whose start or end was an integer too large for a float was skipped when looking for crossings, as its level was until 4049095. Its own check reports the value; the others are still compared with it. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
harmony and mode built a (kind, error) pair for _kept to unpack into a CantCheck; they now build the CantCheck. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The docstrings said CantCheck is raised when no other rule is broken, but the color is checked after it, so that a bad color can't hide that a rule couldn't be checked. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
No check a public function makes gives a kept outcome, so the guard it tested can't be reached from outside, and the test was tied to a private helper. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This was referenced Oct 19, 2026
Python won't turn an integer of more than 4,300 digits into text, so the reason for one, such as a time of 10**5000 from a bulk edit, raised ValueError instead. Reasons show such a value as "an integer too long to show". Files can't hold one: json refuses it. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
TiLiA's get_boundary_conflicts compares boundaries with a 1e-9 tolerance since dev's 2ceec15, because files saved before then hold boundaries a few ulps apart where they should meet. The core compared them exactly, so such a file's hierarchies crossed, and an import skipped rows TiLiA keeps. The test's copy of get_boundary_conflicts is dev's now, and a sweep adds boundaries 1e-13 apart. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
dev's 43a6122 and 2853912 add 13 qualities music21 has no chord type for, each a chord type plus an added tone (dominant-seventh-flat-ninth and so on). TiLiA writes them, and its symbol reading, which the harmony importer moves into the core, makes them. ADDED_TONE_QUALITIES and split_added_tone come into the core, so the list is kept once, and a quality's inversions are counted from its base, as TiLiA counts them. Part of TimeLineAnnotator/project-management#291 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Collaborator
Author
|
🤖 Claude Thanks for the review. The bug and the two differences from
Tests: |
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🤖 Claude
Adds
tilia_core.rules, the one set of rules for a valid component. It covers the kinds the importers create: marker, hierarchy, range, harmony, mode, PDF marker and beat. The importers call it for each row, and later so will bulk edits and the.tlareader and writer. None of them keeps a rule of its own.Rebased onto
v1-prereleaseafter #731 (set up the importers) merged. The rules files are unchanged by the rebase.What changed
importers/_rules.pymoves totilia_core/rules.py. It held TiLiA's creation rules only, and becomes the full set. Its tests move totests/test_rules.py.marker,beat,pdf_marker(withpage_count),harmony,mode,hierarchy,range_(with the timeline'srows). Each takes the media length, the timeline's other components and the attributes to check. It returns the first broken rule as(False, reason), or(True, ""). The result is anOutcome, a 2-tuple whoseothersholds the ids of the other components involved: the one at the same time, the hierarchies it crosses, the next range. It is true when no rule is broken, soif rules.marker(...):reads as "keeps to the rules", and it still unpacks asok, reason.validate_creation, the components'validatorsinset_data, the editing operations, and the format's schema. So the core refuses some values that TiLiA's importers accept:nan;pre_startoutside 0 to its start or apost_endoutside its end to the media length, or crosses another hierarchy (asget_boundary_conflictsdefines it);pre_startis after its start or whosepost_endis before its end, or whosejoined_rightisn't the next range in its row;set_datarefuses (step, accidental, level, display mode, font type, quality, inversion, mode type);Time can't be negative. Got '-00:01.0'), not wrapped around an hour asformat_media_timedoes today.<value> is not a valid <attribute>.. Examples arenan is not a valid start.,1.5 is not a valid level.andnotacolor is not a valid color.. TiLiA's importers word this differently for each kind.not_valid(attribute, value)gives the text, so the importers can give the same one for a value they can't read.color()accepts the default (Noneor""), the hex formsQColoraccepts, and its names, in any case, without spaces or tabs, the way Qt reads them. The names are CSS 3's, read fromwebcolors(webcolors.names("css3")), plus Qt'stransparent: the core keeps no copy of them.webcolors>=24.8.0(pure Python, BSD-3-Clause, no dependencies) becomestilia-core's second dependency; music21 already installs it. A test comparescolor()withQColor.isValid()for every name and for a set of edge cases, when PySide6 is installed.CHORD_TYPESandmodeSharpsAlter. music21 is imported only when a harmony or a mode is checked. Without it, the other rules are still checked. If none of them is broken,CantCheckis raised, namingpip install "tilia-core[harmony]".range_rowschecks a range timeline's rows: their ids, names and colors. Two rows with the same id giveThere is already a row with the id Ab12Cd..The texts for the cases TiLiA has none for were settled in TimeLineAnnotator/project-management#298 (texts for the rules that have none). That issue also settled two behaviours. Text that isn't in NFC isn't refused, since it is stored in NFC. A join to a range that isn't the next one in its row is refused, and when several ranges share the next start, a join to any of them is valid.
Fixes from review
The commits after
b0ada534fix what three code reviews found, one commit per fix. From the first review:Outcomekeeps its reason and itsothersthroughcopy,deepcopyandpickle, andCantCheckkeeps its message throughpickle.OverflowError.1.0.01:00.0, not00:60.0(TimeLineAnnotator/project-management#304 fixes the same in TiLiA).ROW_ID_LENGTHandROW_ID_CHARACTERSare public, and_commit.new_row_idmakes ids with them instead of its own copy.CantCheck, with theImportErroras its cause, instead of raising that error.harmonyextra is declared (harmony = ["music21>=9.1"]), so the commandCantChecknames,pip install "tilia-core[harmony]", installs music21. The default install still has no music21.From the second review:
CantCheck, so an importer would have kept the component unchecked. One helper,_kept, now runs a kind's rules in order, then decides on music21, then checks the color, so every kind gets that order from one place. A step that gives a kept outcome no longer ends the checks.CantCheck, not only onImportError.bool, whateverokwas given as.From the third review:
CantCheck's message, when music21 is found but won't import, gives the reason and the install command, and doesn't claim music21 is installed:checking a harmony needs music21, which can't be imported (ModuleNotFoundError: …): pip install "tilia-core[harmony]". A half-removed music21 (a leftover folder, a missing dependency) gives the same error, and the command fixes it. The text for a missing extra is unchanged.735d4945). A kept failure went stale when a missing dependency was installed, held the failed import's frames, and made the tests depend on their order.check_timelinewill look music21 up once per call instead.CantCheckcopies and pickles whatever its error is. The error isn't carried; its text is in the message.okis aboolhowever it's read, including when unpacked._kepttakes a readyCantCheck, and the docstrings say thatCantCheckcomes before the color._kept's private guard goes.Tests
Run from the worktree's root after
uv sync --all-packages:uv run --package tilia-core pytest packages/tilia-core/tests/test_rules.py: 267 passed, music21 and PySide6 installed.uv run --package tilia-core pytest packages/tilia-core -q: 547 passed before the rebase; 703 after it, onv1-prereleasewith feat(core): set up the importers: CSV reading, results and commit #731 in; 731 with the fixes from the three reviews.python scripts/check_packages.py: all package checks passed, again after the rebase on Python 3.10 and 3.13, and again with the fixes from review. This is a fresh install without Qt or music21, where the harmony, mode and Qt tests skip.tests/test_rules.pyon Python 3.10, without music21 or Qt: 207 passed, 64 skipped (before the last commit, which changed only texts).Each rule the core adds that TiLiA's creation rules look at (times, levels,
pre_start,post_end, nesting) has a test showing that those rules accept the value. Hierarchy nesting is also compared with a copy ofget_boundary_conflictsfor every pair of hierarchies with times from 0 to 4 and levels from 1 to 8. A wider run, levels up to 1000 and fractional times, found no disagreement either.What removing each piece of code did to
test_rules.py:Outcome.__bool__webcolors(onlytransparentleft)pre_start≤ startpost_end≥ endCantCheckwithout music21 (returning OK instead)applied_tojoined_rightis the next rangeOutcome.__getnewargs__CantCheck.__reduce__CantCheckfor a music21 that won't importharmonyextra declared_kept: music21 decided before the color, and a kept step doesn't end the checksCantCheckfor any import failurebool(self[0])CantCheck.__reduce__keeping the message, not the errorbool(ok)inOutcome.__new__Part of TimeLineAnnotator/project-management#291
🤖 Generated with Claude Code