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

Raise an exception if the coercion not supported #4

Merged
merged 1 commit into from
Mar 29, 2013

Conversation

handiwiguna
Copy link
Contributor

coercer[String].to_integer('a')
Coercible::UnsupportedCoercion: Coercible::Coercer::String#to_integer doesn't know how to coerce "a"

@felipeelias
Copy link

That's very useful imo. But I'm not sure if in all cases you'd want this behaviour.

@solnic
Copy link
Owner

solnic commented Mar 29, 2013

@handiwiguna thank you!

@felipeelias it was actually planned to make coercible work in such a strict wait. client libraries (like virtus) can catch those exceptions and silently return input value that couldn't be coerced

solnic added a commit that referenced this pull request Mar 29, 2013
Raise an exception if the coercion not supported
@solnic solnic merged commit bc3ded1 into solnic:master Mar 29, 2013
@mbj
Copy link
Collaborator

mbj commented Mar 29, 2013

@solnic I dislike catching exceptions to steer control flow, cant we have two differend methods? One that raises exceptions and one that doesnt? This would also fit very well in my planned refactoring.

@solnic
Copy link
Owner

solnic commented Mar 29, 2013

@mbj I'm not sure. I need to think about this. I want to keep coercible as simple as possible.

@felipeelias
Copy link

@mbj I think it depends on the purpose of the gem. If you want to coerce a string into a date for example and it fails for some reason, it's much better (imo) to throw an exception than failing silently.

@mbj
Copy link
Collaborator

mbj commented Mar 30, 2013

@felipeelias This is the reason I'd like to see two separate entry points. One that does return the input_, and one that raises an exception_. For both I see valid use cases.

EDIT: * on failure ;)

@solnic
Copy link
Owner

solnic commented Mar 31, 2013

@mbj yeah my initial idea was to make coercible to work in strict and non-strict mode. Maybe let's think how to approach this w/o complicating coercible code too much 😄

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

@solnic There is also a refactoring I was planning. I think this refactoring would allow such an interface easily.

@solnic
Copy link
Owner

solnic commented Mar 31, 2013

@mbj that's cool. So what's your idea?

On niedz., mar 31, 2013 at 9:36 AM, Markus Schirp <notifications@github.com="mailto:notifications@github.com">> wrote:

@solnic There is also a refactoring I was planning. I think this refactoring would allow such an interface easily.


Reply to this email directly or view it on GitHub.

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

@solnic

Primary interface (with exceptions) on failure:

coercer.from(String).to(Integer).call(value)

Secondary interface (returns input on failure)

coercer.from(String).to(Integer).try_convert(value)

# To check for failure
coercion = coercer.from(String).to(Integer)
value = coercion.try_convert("asdsa") # => "asda"
coercion.coerced?(value) # => false

The "coercion" objects returned by coercer.from(Source).to(Target) could be tracked per virtus attribute. Overhead of the new API would be the same as current. (One method call to reach coercion logic).

EDIT: s/path/coercion/

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

@solnic, @dkubb We could also provide a ducktrap compatible API that returns a result object:

coercion = coercer.from(String).to(Integer)

result = coercion.run('foo')
result.successful? # false

result = coercion.run('100')
result.successful? # true
result.output # 100

The use of explicit inversable coercion "path" objects allows different calling semantics quite easily without adding bloat.

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

Last addition (Sorry for rushing out with so many comments)

The "coercion" objects would support #inverse.

coercion = coercer.from(String).to(Integer)

value = "100"

coercion.inverse.call(coercion.call(value)).eql?("100") # => true

This is not only nice for ducktrap, form objects and friends. It also eases testing as transitive properties of the coercible domain could and IMHO should be exploited for correctness!

@solnic
Copy link
Owner

solnic commented Mar 31, 2013

This would be a nice abstraction but I'm worried about a massive overhead it would add.

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

@solnic Yeah the "Returning a Result object" adds a serious amount of overhead.

The other ideas not. If you reuse the "coercion" objects. They do not have mutable state, so are perfect candidates for living inside Virtus attributes. Instead of having a "coercion_method" you just have a "coercer" where you can use #call on.

@solnic
Copy link
Owner

solnic commented Mar 31, 2013

Oh yes I was referring to return objects

Sent from Mailbox for iPhone

On Sun, Mar 31, 2013 at 3:38 PM, Markus Schirp notifications@github.com
wrote:

@solnic Yeah the "Returning a Result object" adds a serious amount of overhead.

The other ideas not. If you reuse the "coercion" objects. They do not have mutable state, so are perfect candidates for living inside Virtus attributes. Instead of having a "coercion_method" you just have a "coercer" where you can use #call on.

Reply to this email directly or view it on GitHub:
#4 (comment)

@mbj
Copy link
Collaborator

mbj commented Mar 31, 2013

I think this returning objects API is fast enough for parsing forms. It should be up to the user to decide which one he uses. I'll not implement the "Result Objects" in my first refactoring.

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.

4 participants