Closed jonolsson closed 7 years ago
@jonolsson thank you so much the PR. It looks great, just one little thing, I think we do not need to pull dry-container as a dependency since a container can just be something that respond to 'container[:element]' like a hash, you can check the rest of the spec for an example https://github.com/jonolsson/dry-transaction/blob/d3eed02163193235d6f331804ca04e0c5c21f327/spec/integration/transaction_spec.rb#L17
Great. The reason I pulled in dry-container as a dependency was that dry-container behaves slightly different then a hash for calls to container[:element]. Which was the the problem to begin with. So by adding it I could start with a failing test. But as container.key?(:element) has the same behaviour for both dry-container and hash dropping the dependency should be fine. I have removed that and updated the PR.
This looks good to me. What do you think @timriley ?
@GustavoCaso not completely sure about what you mean by "dependency", but dry-container is already a dependency of dry-transaction.
With that said, I'm with @jonolsson: this PR only exists because it's possible to use containers with dry-transaction. Repeating what I wrote in #61, IMHO, specs should be using a Dry::Container instance as the container instead of a hash because, AFAIK, no example in the documentation uses a hash as the container.
Hi @jonolsson – thanks for your PR and for looking into this issue!
I've just been able to take a closer look at this now and I'm definitely still happy with your fix, but I think it'd be great if we can tighten up the specs a little – for example, if I reverse your change to operation_resolver.rb
, your new spec still passes.
Like you pointed out, this is because we've been using hashes in our specs instead of dry-container instances. I think the real answer here is to replace those hashes in the specs with dry-containers. Would you be up for giving that a go within this PR?
Thanks!
@jonolsson @jcmfernandes both were absolutely right about adding the dry-container
and using dry-container
in the test to have the test reflect as much as possible how the lib is used. Sorry for misleading comment ❤️
Sure I'll give it a go.
Thanks @jonolsson!
Released in 0.10.2
PR for Issue #61 It adds a new dependency for the tests to dry-container and and a new additional container to the tests, I hope this additions are not to far away from the conventions you have for this.