Check if an operation is registered in the container before getting it (Issue #61)

[jonolsson]

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.

[GustavoCaso]

@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

[jonolsson]

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.

[GustavoCaso]

This looks good to me. What do you think @timriley ?

[jcmfernandes]

@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.

[timriley]

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!

[GustavoCaso]

@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 ❤️

[jonolsson]

Sure I'll give it a go.

[jcmfernandes]

This isn't required as it's already part of the gemspec.

[timriley]

Thanks for pointing this out @jcmfernandes, but I'm thinking we don't actually need it in the gemspec, since dry-container is a "soft" dependency. I'll probably be removing the gemspec dependency before the next release, so I'm happy for this to stay here :)

[jcmfernandes]

Good point @timriley, indeed, it makes more sense to have it here. As you stated, dry-container isn't a runtime dependency.

[timriley]

Thanks @jonolsson!

[timriley]

Released in 0.10.2