stevilknevil: Just a cautionary note that zas is also working on refactoring in the metadatabox files. Maybe mostly done with https://github.com/metabrainz/picard/pull/2608 , not sure :) Just to be aware that there could be merge conflicts with changes you are working on
stevilknevil[m]
@outsidecontext: Yep - fortunately I'd spotted that, so I'm going to hold fire for now! Either that or branch from there 🙂
zas[m]
the refactor patch was just merged, so it will be easier
outsidecontext[m
zas: you are fine with merging stevilknevil 's short patch? Only thing is that we should squash the commits for this short patch. But if there are no other changes we can do this in github
zas[m]
outsidecontext: yes, after a squash, but more work is needed around this (as discussed)
we can just merge the "fix", and then work at improving the code so the fix isn't really needed anymore
outsidecontext[m
yes, agreed. but for fixing the crash this is fine
zas[m]
I totally agree there's no point in handling ~length differently
outsidecontext[m
stevilknevil, zas : all is merged
Protopia[m] joined the channel
Protopia[m]
<stevilknevil[m]> "Morning all! I've been tinkering..." <- 1. I can see why you might want to put the original value in the JSON in order to confirm that you are replacing the correct value, but that would be a very specific use case and not the general one where only the new values are needed. The JSON should be simplified... (full message at
outsidecontext: we should clarify format_time(), to me it expects milliseconds and positive integer, https://github.com/metabrainz/picard/blob/0d864... doesn't test everything, it would happily accept to convert -100.23 (we can accept floats and/or negative numbers of course, but then we need to ensure the behavior is correct)
outsidecontext[m
Yes, should be clarified and tests expanded. What does it currently do if a negative value is passed? IMHO it should fail
Currently: floats get round90ed. and negatives return something like -1:20
s/round90/`round()`/, s/-/`-/, s//`/
outsidecontext[m
I think float is not so much of a problem. It is fractions of milliseconds, and the smallest unit displayed is seconds anyway.
Maybe have a test with a float to both test and document the expectation
stevilknevil[m]
On reflection, I agree with you about the floats being a problem. passing in 0.1ms is not really worse than passing in 1ms when we're returning with fidelity of 1s anyway
* the floats not being a
I saw some docs about configuring pre-commit hooks in my dev environment, but I can for the life of me find it now - any pointers?
I'd literally just been reading that! How did I not see it?! hehe
zas[m]
btw, I have no problem format_time() works with negative and floats, it could be handy, but it has to be clear it supports those (we could even add accept_negative=False keyword argument, so it errors on negative by default), but for sure those cases have to be tested (and documented)
stevilknevil: you can also use ruff (uvx ruff check), but isort will catch some extra issues with imports order & style
stevilknevil[m]
bleugh - windows dev environment, I'll need to do some fiddling and/or use wsl!
zas[m]
switch to Linux, problem solved
stevilknevil[m]
Exactly 🙂
I like to make things harder for myself
Under what circumstances (in the UI) would the tag_diff return more than one string? If I add multiple genres to the same track, then they all get concatenated to a single string like `["genre1/genre2"]` if I have multiple files selected with different genres then I get a blank array of strings `['']` - what am I misunderstanding?
zas[m]
It doesn't, it just checks for changes, if there's more than one, it just says how many, but do not keep values
Basically it counts objects that differs, but do not keep track of values when there's more than one
It is for display purpose only. If you want actual values look at File or Track metadata & orig_metadata. The current code was meant to work with single values and for that TagDiff was working. But imho that's not really correct.
