Added logic for utilising mkvpropedit with pymkv2

[MasinAD]

I already used pymkv2 in a script to mux several files into one so it was a logical choice for me to also use it to edit properties of an existing MKV file. I was puzzled by the lack of the functionality so I hacked it together myself and want to contribute it back.

I tried to orientate myself at how things were already done but saw the necessity to refactor some small parts to make it work easier with mkvtoolnix' approach for mkvpropedit. I have neither checked if I broke mkvmerge usage nor did I write any tests due to lack of knowledge on how to write tests (or if there are even tests for pymkv2).

I'd gladly like to iterate on this pull request.

What does it do? Wherever possible I hooked into existing code to record changes made to an MKVFile instance and its MKVTrack children. These changes are then compiled into an mkvpropedit command line in MKVTrack.propedit() and called from MKVFile.update(), the latter being basically a copy of MKVFile.mux().

I restructured MKVTrack to store the MKV flags in a _flags dictionary and re-created object properties with the former names for backwards compatibility. If there's any breakage to be expected I'd guess the reasons lie there.

The list of MKV properties mkvpropedit is able to handle is much longer than whatever pymkv2 supported, and I only implemented code for those properties I needed and those pymkv2 already handled for mkvmerge. Extending this beyond the current scope is easy, though.

[codecov]

Codecov Report

❌ Patch coverage is 59.05512% with 52 lines in your changes missing coverage. Please review.
✅ Project coverage is 65.53%. Comparing base (a4b21e9) to head (918d9da).

Files with missing lines	Patch %	Lines
pymkv/MKVFile.py	20.83%	38 Missing ⚠️
pymkv/MKVTrack.py	83.07%	11 Missing ⚠️
pymkv/Verifications.py	78.57%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   67.25%   65.53%   -1.73%     
==========================================
  Files          10       10              
  Lines         959     1062     +103     
==========================================
+ Hits          645      696      +51     
- Misses        314      366      +52     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[GitBib]

Thank you for the first PR. I’ll review it and get back to you later.

[MasinAD]

I tried to understand the code coverage tests but I don't get it. I understand there are lines not hit during tests, I found the tests and read the GitHub workflows to be able to run the tests locally, but I don't know how to extend those tests to cover the added lines, sorry.

[GitBib]

@MasinAD To understand how to work with the tests, start by studying the Makefile file — it prepares the data and runs the tests. All the tests are divided according to the main actions you can perform with pymkv.

[GitBib]

I deliberately didn’t use the if ... in ... else None construction because it would be difficult for other people — and even for you yourself — to understand what’s going on there.

[GitBib]

I’d move the cmd-related logic out of update, following the approach used in the main code.

[MasinAD]

I deliberately didn’t use the if ... in ... else None construction because it would be difficult for other people — and even for you yourself — to understand what’s going on there.

I did that because I need a way to distinguish between initialization of the object and later modifications. Otherwise the _property_changes fill up with all those initialization assignments that aren't needed. I can and will revert it but need to find another way of telling the object not to record the changes.

[MasinAD]

I’d move the cmd-related logic out of update, following the approach used in the main code.

I used the functions mux() and command() as models for my functions update() and propedit(). update() is basically an adaptation of mux() while propedit() fills the role of command(). I didn't see a good way of hooking into existing code. At least, that's where I see the "main code". Or did I miss something? 🤔

I'd really like to merge my contributions into the existing functions but I don't want to risk compatibility. I'd rename command() to merge() or merge_command() and if the latter propedit() to propedit_command(). update() and mux() could be united to something like run_command(), taking any of the former functions as an argument.

[MasinAD]

I tried adding this test test_flags.py:

from pathlib import Path

from pymkv import MKVFile


def test_set_flag_default_true(
    get_base_path: Path,
    get_path_test_file: Path,
) -> None:
    mkv = MKVFile(str(get_path_test_file))
    track = mkv.get_track(1)
    track.default_track = True
    assert track.default_track

def test_set_flag_default_false(
    get_base_path: Path,
    get_path_test_file: Path,
) -> None:
    mkv = MKVFile(str(get_path_test_file))
    track = mkv.get_track(1)
    track.default_track = False
    assert not track.default_track

But pytest always complains about

Item "list[MKVTrack]" of "MKVTrack | list[MKVTrack]" has no attribute "default_track" [union-attr]

I'd have added similar test for all the other flags but I refrained from it until I sorted this one out. To me it seems like pytest does not like class properties and assumes they wouldn't exist.

Currently, that's a bit of a showstopper for me as I cannot easily test the command_propedit() and run_command() functions if I don't have any means to make any changes to an MKVFile without pytest complaining.

When using the above code as a simple python script, it works without any issues:

from pathlib import Path

from pymkv import MKVFile


def test_set_flag_default_true(
    get_base_path: Path,
    get_path_test_file: Path,
) -> None:
    mkv = MKVFile(str(get_path_test_file))
    track = mkv.get_track(1)
    print( track.default_track )
    track.default_track = True
    assert track.default_track

test_path = Path.cwd() / "tests"
test_file = test_path / 'file.mkv'

test_set_flag_default_true( test_path, test_file )

Any advice?