Refactoring the disk cache to have directory as an init parameter#78
Conversation
Running calculations of scores & best scores over several directories turned out to be complex if directory was passed as a call parameter. This refactoring makes it part of the init parameters of the optimizer, the context and the cache. This means that one has to have functions for "adopting" an encoding into the other context. Fixes #77
| return 'worsened' | ||
|
|
||
| def VerifyOneTarget(codec_names, rate, videofile, score): | ||
| def VerifyOneTarget(codec_names, rate, videofile, old_scores, score): |
There was a problem hiding this comment.
My standard comment: You are refactoring untested code. 2 risky aspects to the refactoring: 1. splicing in a new parameter between old ones. 2. Spitting up the role of my_optimizer into 2 objects, old_optimizer and new_optimizer.
Not sure if there is a good way to test this, but since the tests you wrote in the past so secure similar-sized changes actually caught bugs, I probably should at least raise this.
There was a problem hiding this comment.
Acknowledged. The naming structure I've adopted makes it hard to write tests for code in binaries (no .py means that you can't "import" them into the test).
The bug that this fixes is that before, if a score got worsened, it would result in it disappearing from the list of possible best scores, which meant that the next run would test the change against a different score :-(
This was part of #82 - but I decided to separate the code work from the actual version flip.
My only mitigating point on the "insert parameter in the middle of list" is that the function is called from exactly one place, and because of the naming mentioned above, I'm assured that it's not imported anywhere else :-)
There was a problem hiding this comment.
All fair enough. Fix for the hard-to-test binaries is of course to move test-worthy code to lib. Granted, doesn't help as a safety net even for that bin2lib refactoring but might provide a motivation going forward to be more strict than in the past about allowing actual work code to live in the binaries directly.
|
PTAL. Not moving code between bin and lib at this time. |
|
LGTM |
3 participants
Running calculations of scores & best scores over several directories
turned out to be complex if directory was passed as a call parameter.
This refactoring makes it part of the init parameters of the optimizer,
the context and the cache.
This means that one has to have functions for "adopting" an encoding
into the other context.
Fixes #77