search

martinklepsch / s3-beam

🚀 direct-to-S3 uploading using ClojureScript
Eclipse Public License 1.0
92 stars 17 forks source link

upgraded signing to aws version 4 #36

Closed whamtet closed 7 years ago

whamtet commented 7 years ago

Dear Martin,

thanks for the great library. It now supports aws version 4 which works in all regions, not just the US. If you want any other improvements before accepting the pull request, let me know.

Matt

whamtet commented 7 years ago

Since you're so picky you can upgrade to version 4 yourself. My two main suggestions are

1) The file needs to be the last parameter, that's S3's decision, not mine. 2) Don't import a library to replace a one line function.

martinklepsch commented 7 years ago

@whamtet Daniel just wanted to be helpful, I don't think "being picky" was his objective. He gave you valid feedback about your changes, from formatting to just general improvements and proper usage of language features.

Your changes also deleted a previously needed function without further explanation why.

You can step back now and say you don't want to put in the effort to make it right or you can learn from Daniel and improve the PR. Daniel thoroughly reviewed your code, which is something I would be thankful for instead of downplaying it as "the reviewer is being picky".

I will lock this Pull Request. If you decide to revise your PR please open another one and be respectful towards people offering their free time to review and integrate your code.