search

fmang / opustags

Ogg Opus tags editor
BSD 3-Clause "New" or "Revised" License
75 stars 10 forks source link

Add support for multiple input files with --in-place (fix #17) #26

Closed rrthomas closed 4 years ago

rrthomas commented 4 years ago

Thanks very much for your feedback; I didn't expect the PR would land first time! I'll see what I can do and update the PR.

rrthomas commented 4 years ago

This current implementation breaks --set-all for multiple files, because opustags reads its tags from stdin while it is processing the Opus file. To support multiple files, we need to read the tags at once into memory before processing the Opus files. That probably deserves a separate pull request before we can merge this one.

I will look at this next.

It is also missing test cases in opustags.t.

Added.

Such a feature raises a few other questions:

1. What to do when the same file is specified twice? I think it is reasonable to edit it twice. That’s what sed does.

I agree; and this will happen automatically.

2. What to do when one file fails? Shouldn’t we continue?

Done!

3. What should be the error code if some files succeed but other fail? Right now opustags returns either 0 or 1, so I guess 1 is better when there is a failure.

Done!

rrthomas commented 4 years ago

How is the Perl test not useful?

rrthomas commented 4 years ago

I think I've dealt with all your comments; thanks for the review!

fmang commented 4 years ago

How is the Perl test not useful?

It doesn’t check that out2.opus was indeed processed. A better test would be to have two files with different tags, perform an incremental operation like --add, then check the content of both files. You can see most other tests check the MD5 because that’s easy. By the way, calling opustags to copy gobble.opus to out.opus is a bit awkward, Perl has copy for that. Still about style: no need to unlink out2.opus at the beginning of the file since we’ll overwrite it anyway. If you do want to unlink it, unlink it near where it’s needed.

I think you forgot to push your changes. Please also rebase them on the new master.

rrthomas commented 4 years ago

Sorry, I pushed my changes to a different branch. Now pushed to this branch.

rrthomas commented 4 years ago

I updated the test as suggested.

rrthomas commented 4 years ago

Many thanks! And thanks once again for writing opustags…I must file a bug to ask for it to be packaged in Debian!

fmang commented 4 years ago

Merci à vous !