Conversation
…live bug Working #350's tail. mypy goes from 90 errors in 16 files to 76, and as with the earlier passes the errors were failures rather than typing debt. *A weak reference dereferenced unguarded.* `Flow` holds its `MgVideo` weakly, so that the video holding the flow and the flow holding the video do not make a cycle. Nothing therefore keeps the video alive in `flow = mg.MgVideo(p).flow`, and every later `self.parent()` resolves to None. Calling `flow.dense()` after that raised `AttributeError: 'NoneType' object has no attribute 'flow_dense_video'`, which names the attribute reached for and says nothing about the cause. Verified on the parent commit before changing anything. The fifteen dereferences go through a `_parent()` resolver now, which raises a sentence showing the pattern that works. Seventeen errors in that file become seven. *A frame rate is not an integer.* `frame2ms` declared `fps: int` while every caller passes `self.fps` read from the file. Commit fa0f719 made that rate a true float on the grounds that truncating it is wrong; this signature never followed. *`mg_motiondata` said it returned a list.* It returns the path it built: a `str` for one data format and a `list` for several, which I confirmed by running it. An unsupported `data_format` also says so now instead of returning None after doing all the work. Five tests for the weak reference, one of them pinning that it stays WEAK --- a well-meaning change to a strong reference would make every video that touched `.flow` immortal. DELIBERATELY LEFT for ARJ: `self.fps` is `float | None` and accounts for about eight of the remaining errors. `get_video()` always replaces it with the real rate, so the attribute is genuinely a float after construction, but saying so means changing a guard in `__init__` and that changes behaviour when `fps=0` is passed explicitly. Not a call to make unattended. 680 tests pass, 4 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ARJ's call. The ARGUMENT stays optional, because a file carries its own rate and only an array needs one supplied. The ATTRIBUTE is not: `get_video()` reads the true rate from the file and overwrites whatever the constructor stored, so by the end of `__init__` it is always a number. Saying so closes twelve errors across the modules that divide by it --- mypy goes from 76 to 64 --- and it was also simply true, which is the better reason. The edge case I had flagged as needing a decision turned out not to exist. I built the parent commit in a scratch worktree and ran it: `MgVideo(array=arr, fps=0)` raised `FileNotFoundError: z.avi` there, exactly as it does now. The from-array branch tests the rate for truth rather than for None, which rejects an explicit zero, and zero was never a frame rate anything could encode at. Both spellings blamed the output file for a missing argument, though, because the encode never ran and the read after it found nothing. So the case raises a ValueError naming the real problem instead. 682 tests pass, 4 skipped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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
1 participant
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.
Working the tail of #350. mypy goes from 90 errors in 16 files to 76, and as in the earlier passes the errors turned out to be failures rather than typing debt.
A weak reference dereferenced unguarded
Flowholds itsMgVideoweakly, so that the video holding the flow and the flow holding the video do not make a cycle. Nothing therefore keeps the video alive in this shape, which does not look wrong:Every later
self.parent()resolves toNone, and the call raisedAttributeError: 'NoneType' object has no attribute 'flow_dense_video'— naming whichever attribute was reached for and saying nothing about the cause. Reproduced on the parent commit before anything was changed.The fifteen dereferences now go through a
_parent()resolver that raises a sentence showing the pattern that works. That file goes from seventeen errors to seven.A frame rate is not an integer
frame2msdeclaredfps: intwhile every caller passesself.fpsread from the file. Commitfa0f719made that rate a true float on the grounds that truncating it is wrong; this signature never followed.mg_motiondatasaid it returned a listIt returns the path it built: a
strfor one data format and alistfor several, confirmed by running it. An unsupporteddata_formatnow says so rather than returningNoneafter doing all the work.Tests
Five, for the weak reference. One pins that the reference stays weak — a well-meaning change to a strong one would make every video that touched
.flowimmortal.680 tests pass, 4 skipped, locally on Linux/Python 3.12. This PR exists to get the full matrix, since CI does not run on branch pushes.
Left deliberately
self.fpsisfloat | Noneand accounts for about eight of the remaining errors.get_video()always replaces it with the real rate, so the attribute really is a float after construction — but saying so means changing a guard in__init__, which changes behaviour whenfps=0is passed explicitly. That is a call for @alexarje rather than one to make unattended.🤖 Generated with Claude Code