Closed srfraser closed 6 years ago
MihaiTabara
commented
6 years ago Thanks for working on this! First things first, I think we need to make the tests green. Most of the code should be adding, so existing tests shouldn't be affected whilst new code should have its own. I vaguely remember this PR used to be green by Lisa's end of internship but I could be wrong. I'll dig into it too when I review.
srfraser
commented
6 years ago The tests were all green locally, looks like I missed a mar file from the upload. I don't think it's true that things were mostly added, there's a bit of refactoring in move_beets and similar, too.
srfraser
commented
6 years ago ahh:
The following paths are ignored by one of your .gitignore files:
beetmoverscript/test/test_work_dir/cot/eSzfNqMZT_mSiQQXu8hyqg/public/build/fake-99.0a1.en-US.partial.mar
Use -f if you really want to add them.
coveralls
commented
6 years ago | Changes Missing Coverage | Covered Lines | Changed/Added Lines | % | ||
|---|---|---|---|---|---|
| beetmoverscript/task.py | 32 | 34 | 94.12% | ||
| beetmoverscript/script.py | 50 | 55 | 90.91% | ||
| <!-- | Total: | 93 | 100 | 93.0% | --> |
| Files with Coverage Reduction | New Missed Lines | % | ||
|---|---|---|---|---|
| beetmoverscript/script.py | 1 | 97.94% | ||
| <!-- | Total: | 1 | --> |
| Totals | |
|---|---|
| Change from base Build 706: | -0.3% |
| Covered Lines: | 718 |
| Relevant Lines: | 727 |
MihaiTabara
commented
6 years ago @srfraser ah, nice! Hm, I'll (re)glance when I review this to see what "modified". If this was ready to go live to production, we'd need to support both old behavior and new behavior whilst the manifest rides the trains. So there might be some code at that intersection that needs to toggle between them, hence modified. But other than that, afaik it shouldn't. Will reply more consistently once at review time which I hope to get done later on today or tomorrow morning. Thanks again for the help with this!
I think I've caught all the changes required to successfully merge this.