search

taoensso / faraday

Amazon DynamoDB client for Clojure
https://www.taoensso.com/faraday
Eclipse Public License 1.0
238 stars 84 forks source link

New feature: projections on get-item and query, extra query functionality, new tests #76

Closed ricardojmendez closed 8 years ago

ricardojmendez commented 8 years ago

Adding support for projections, as well as tests. Did it against 1.8.0 instead of the 1.9 beta from dev because dev did not pass tests for me.

ricardojmendez commented 8 years ago

Actually, let me go over these changes a bit more. I'm not really liking the parameter names, and may want to do some other additions. Closing the pull request for now.

ricardojmendez commented 8 years ago

Re-opening, now that I've renamed the parameters to be more readable when possible, and to be consistent with #73 when there was a precedent. Also added extra functionality to query and expanded on the tests.

ptaoussanis commented 8 years ago

Hi Ricardo,

Thanks so much for this! Looks good, but this'll indeed unfortunately need to target the dev branch (v1.9 beta). Could you let me know what failures you were seeing so we can get them sorted out?

Thanks,

ricardojmendez commented 8 years ago

Hi Peter,

Here's the error: 

$ lein test
Reflection warning, expectations/platform.clj:39:23 - reference to field getMessage can't be resolved.
Reflection warning, expectations/platform.clj:58:3 - reference to field printStackTrace can't be resolved.
Reflection warning, expectations.clj:131:53 - reference to field getStackTrace can't be resolved.
Reflection warning, expectations.clj:566:16 - reference to field ns can't be resolved.
Reflection warning, expectations.clj:566:28 - reference to field sym can't be resolved.
Exception in thread "main" java.lang.RuntimeException: Unable to resolve symbol: expect-let in this context, compiling:(taoensso/faraday/tests/main.clj:315:1)
    at clojure.lang.Compiler.analyze(Compiler.java:6543)
[...]

Seems to be an issue with the version of expectations.

If you want to merge these changes into master, I'll be happy to help bring them to dev as well once that's fixed.

ptaoussanis commented 8 years ago

Thanks, have reverted the relevant dependency bump - any chance I could ask you to try again against dev HEAD? Should be okay now.

If you want to merge these changes into master, I'll be happy to help bring them to dev as well once that's fixed.

Thanks, my preferred merge process is for all development work / PRs to target the dev branch. Then when cutting a release, I'll pull everything into master (which should automatically be a clean merge), bump the version+changelog, and publish a release.

:-)

ricardojmendez commented 8 years ago

Sure, going to rebase them and we'll find out how github's pull requests behave with rebased changes.

ricardojmendez commented 8 years ago

Rebased to dev, deleted commit with the version number bump. Pull request looks as expected, pretty neat that Github actually supports it.

ptaoussanis commented 8 years ago

Great, thanks- merging now to take a closer look

ptaoussanis commented 8 years ago

Okay, have merged your changes into the dev branch and pushed [com.taoensso/faraday "1.9.0-SNAPSHOT"]. This is great work, thank you.

I'll let you accumulate any other changes on the dev branch; you can let me know when you're ready and I'll cut a stable v1.9.0-alpha3 release.

ricardojmendez commented 8 years ago

Sounds good, will do!