Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add initial rules context interface #675

Merged
merged 2 commits into from
Nov 23, 2020
Merged

Add initial rules context interface #675

merged 2 commits into from
Nov 23, 2020

Conversation

pyromaniac
Copy link
Contributor

Implements #674

@pyromaniac
Copy link
Contributor Author

There is a problem with ruby 3. Looks like I have to wrap all the params passed to Contract#call with curly braces. This is actually going to be a breaking change for ruby 3. What do you think would be the best course of action here?

@pyromaniac
Copy link
Contributor Author

And if I use context arg instead of kwarg - the problem is gone.

@solnic
Copy link
Member

solnic commented Nov 12, 2020

@flash-gordon could you suggest what to do? I'm surprised doing def call(input, context: EMPTY_HASH) breaks in ruby 3.0 to be honest.

@flash-gordon
Copy link
Member

It breaks in cases like

age_contract.(age: 17)

They have to be rewritten as age_contract.({ age: 17 }) which in my opinion makes perfect sense. Practically speaking, it won't be a problem since you usually pass hashes as variables to contracts. TBH I'm not sure which path we want to choose here, keywords seem more extensible, and updating all specs with hash literal syntax is not a problem. @solnic wdyt?

@solnic
Copy link
Member

solnic commented Nov 19, 2020

How about something a bit more radical:

def call(input = nil, input:, context:)
  # ...
end

Reasoning: forcing people to use literal hash syntax in #call would be bad experience IMO

Alternatively, we could mimic what dry-validation 0.x used to do - have a default context established via params and expose Contract#with (which is really a standard way across dry/rom libs). Downside: providing the context would create a new contract instance.

@pyromaniac
Copy link
Contributor Author

2.6.6 :001 > def call(input = nil, input:, context:)
2.6.6 :002 >   p input
2.6.6 :003 > end
SyntaxError ((irb):1: duplicated argument name)
def call(input = nil, input:, context:)
                      ^~~~~~

@flash-gordon
Copy link
Member

flash-gordon commented Nov 19, 2020

Reasoning: forcing people to use literal hash syntax in #call would be bad experience IMO

I don't think there's much evidence supporting this. Besides, this is a change in the language, not some fancy stuff we added.

Alternatively, we could mimic what dry-validation 0.x used to do - have a default context established via params and expose Contract#with (which is really a standard way across dry/rom libs). Downside: providing the context would create a new contract instance.

Yeah, this would work. We would have to use option :context, default: Undefined + context = Undefined.default(self.context) { EMPTY_HASH.dup } in #call so that we don't share the same context between contract calls.

@solnic
Copy link
Member

solnic commented Nov 19, 2020

@flash-gordon yeah with option-based approach we'd also support both use cases where you want to have a predefined context for all contract instances AND still be able to provide a predefined context at run-time. If it turns out to be too slow for some reason, we could revisit this in 2.0.0. WDYT @pyromaniac ?

@flash-gordon
Copy link
Member

@solnic if you go with predefined context you'll need to copy it in #call. Maybe we need to check if it's frozen in #initialize. Just an important detail, otherwise people will certainly share the context object across contract instants, there's no way around it :)

@pyromaniac
Copy link
Contributor Author

Actually, the current implementation deals with it. Both initial contexts get kind of duplicated on Concurrent::Map initialization.

@pyromaniac
Copy link
Contributor Author

I also can't quite say that I understood what are you folks trying to achieve.

@solnic
Copy link
Member

solnic commented Nov 23, 2020

I also can't quite say that I understood what are you folks trying to achieve.

In rom-rb and probably some dry-rb libs, we have a standard way of providing options to an existing instance via method called #with, so here, rather than extending #call, I thought that we should use #with instead BUT it does not matter as we don't actually have it available here. Adding it is a bit tricky so I'd prefer to hold off until 2.0.0.

Having said that, I think the PR is good as-is, it adds the feature with a minimum effort so I'm gonna merge it in 🙂 Thanks for working on this!

@solnic solnic merged commit 06cdd20 into dry-rb:master Nov 23, 2020
@pyromaniac pyromaniac deleted the initial-context branch November 23, 2020 18:21
@pyromaniac
Copy link
Contributor Author

I see now, thanks for the explanation. This with approach actually makes sense to me. But still sounds like a mix-up of compile-time and runtime features to me. The things that are going to the context in my practice, they don't really belong to the instance state. They are short-living (like domain entities for example). On the other hand, this is just a matter of the paradigm expressiveness. With OPP we basically have at most 2 stages for running something (prepare context during the object initialization and having additional data passed on the instance method call). In FP we have as many stages as we wish with currying but it is not easy to design functions allowing to modify any stage out of order. Trade offs are everywhere :)

@solnic
Copy link
Member

solnic commented Nov 23, 2020

@pyromaniac I think it could be useful, ie you could inject some default settings specific to your app and then still have the ability to override them at run-time. This has proven to be useful in other libs, that's why I thought about it here too.

@pyromaniac
Copy link
Contributor Author

Yeah, sure, that's why I implemented both in this PR as you suggested, it makes total sense. I'm talking about with, which is going to store basically everything in the object state.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants