search

russmatney / lambda-mp4s-to-timelapse

converts files from s3 into a timelapse with endcard and music
3 stars 4 forks source link

move binaries to root, change stitch-mp4s to except unlimited mp4s #2

Open stfnhh opened 9 years ago

stfnhh commented 9 years ago
russmatney commented 9 years ago

Thanks for this! I'll take a look in the next day or so – can you speak to the reasons behind the changes? It'll help me understand how I should think through your code.

The API change is certainly more flexible, but it'll break some stuff I have relying on this. That's covered by version control of course, but it'd be nice to include a changelog for people who need to update.

stfnhh commented 9 years ago

Added the changelog to README.md

stfnhh commented 9 years ago

Have you had a chance to review these changes?

russmatney commented 9 years ago

Sorry for the delay – I have not had a second, thought I'd be able to create some more time for this by now. I had a few things right off the bat that I wanted to consider more before pressing for detail/pushing back on, namely the moving of the binaries and breaking the current api vs. leaving both interfaces accessible.

Moving the binaries might be the right thing, but feels like unnecessary work – I've used this structure for all my Lambda functions without any issue. Feels more like preference than not (hence, I want to look more into why you did it, maybe it's better this way).

I don't mind an improvement to function's signature - but I also don't want to create more work that I won't have time to do, i.e. going back to the functions that rely on this to update and re-upload them. Version pinning isn't really an option here - it's not the same as a module's dependency, and uploading this function would break production for the places that are using it.

I'll make a point of creating time on Sunday to resolve as much of this as I can - thanks much for the PR and for being patient and persistent.