search

lisamelton / video-transcoding-scripts

Utilities to transcode, inspect and convert videos.
MIT License
603 stars 76 forks source link

Simplify getting output file name without extension #11

Closed vitorgalvao closed 9 years ago

vitorgalvao commented 9 years ago

Changes the burden from sed to bash itself. Not sure why you took different approaches with these, though, was it intentional?

lisamelton commented 9 years ago

Believe it or not, that whacky sed command is in transcode-video.sh to fix a bug with directory, rather than file, input. I just didn't add a checkin or source comment to explain it. My bad.

It seems that some people are either downloading oddly named disc image directories or using strange tools to create them. Anyway, the bug report (via Twitter, I think) said that transcode-video.sh was deleting portions of the name, so that sed command is a hack to detect a valid filename extension without screwing up directory names containing periods. And, so far, it seems to work.

So, a simple Bash expression, albeit easier to read, wouldn't work in that case.

However, since convert-video.sh doesn't take directory input, your change there is probably fine. And a good idea, too. I just need to test it.

Can you break these up into two pull requests? Or just remove the pull request for transcode-video.sh?

lisamelton commented 9 years ago

BTW, if you can think of a better, less "hacky" way to fix that bug I was working around in transcode-video.sh, then I'm open to suggestion. I wasn't thrilled with the workaround when I wrote it.

vitorgalvao commented 9 years ago

Won’t that prevent mistakes only when the directory contains a period and the text after it contains special characters? Meaning a directory with a period and only alphanumeric characters after it would still get those stripped of.

Why not use a simple conditional to set the output name depending on if the input is a file or a directory? It should be more reliable. There’s a suggestion in #13.

I’ve also updated this one to make the change only to convert-video.sh

lisamelton commented 9 years ago

Yes, there are probably some edge cases where the hack doesn't work.

But there's one case (which I stupidly didn't mention earlier) that the current code does handle for directories -- when they actually do have an extension. This is typical using "RipIt.app," probably the most popular DVD ripping program on OS X. RipIt creates directories ending in .dvdmedia and users were also complaining this wasn't being stripped.

And, unfortunately, the suggestion in https://github.com/donmelton/video-transcoding-scripts/pull/13 doesn't handle the .dvdmedia case.

Anyway, thanks for isolating the change to convert-video.sh. I'll test and merge that sometime today.

lisamelton commented 9 years ago

OK, that's in. Thanks again!