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

Feature: Add default result to try method #3650

Conversation

dennisjbell
Copy link

Rational:
To facilitate chaining of method calls without successive try
wrappers, the addition of a default value to the try call that is
returned when the receiver is nil allows the returned type to be made
homogeneous.

my_string_or_nil.try(&.size)    # => could be Nil or Int32
my_string_or_nil.try(0, &.size) # => will always be Int32

While this is similar to (my_string_or_nil.try(&.size) || 0), the
use of a default in try more elegantly solves the case when the result
could be false or nil.

Copy link
Member

@bcardiff bcardiff left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the addition. It is aligned with Hash#fetch(key, default) actually.

  1. (mandatory) run the formatter, otherwise travis will complain.

  2. (opinion) I would leave a single Nil#try method with the same default as Object. The default would be duplicated but it is easier to follow I think. WDYT?

  3. (opinion) rename if_nil_value to default so it is the same as Hash#fetch.


it "yields self when not nil even when default given" do
obj = "An arbitrary string"
obj.try("another value") {|o| o}.should eq(obj)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

{|o| o} -> &.itself

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, I did that idiomatic crystal in other examples, but wanted to show that this way resulted in the same behaviour

obj = /.* (.)(.)(.)/.match("An arbitrary string")
typeof(obj).should eq(Regex::MatchData | Nil)
typeof(obj.try(&.size)).should eq(Int32 | Nil)
typeof(obj.try(0, &.size)).should eq(Int32)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@dennisjbell
Copy link
Author

Ironically, it was originally named default, but I wanted to indicate that it was only used if the value was nil. Will change it back to default, as well as the other items you identified.

@dennisjbell
Copy link
Author

@bcardiff Do you want me to flatten the PR to one commit, or keep each commit separate?

@asterite
Copy link
Member

asterite commented Dec 7, 2016

exp.try(&.method) || other is actually better because other will be lazily evaluated. I personally don't feel the need to add a default value as an argument. For the case of a boolean, with try you are already getting a truthy/falsey value, which is what you ultimately care about with booleans.

@asterite
Copy link
Member

asterite commented Dec 7, 2016

(also the above is a strong idiom in Crystal, the usage of ||)

@matiasgarciaisaia
Copy link
Member

@asterite wouldn't that misbehave if false or nil are valid .method return values?

@bcardiff
Copy link
Member

bcardiff commented Dec 7, 2016

An alternative would be to have something as Object/Nil#unnil(&block) so
my_string_or_nil.try(&.size).unnil { 0 } to allow laziness for the default value.

But I think the proposed change to Object/Nil#try is consistent with Hash#fetch.

@kostya
Copy link
Contributor

kostya commented Dec 7, 2016

problem with || that need to add () when chains methods:
asdf.(exp.try(&.method) || other).asdf

seems better:
asdf.exp.try(&.method).else { other }.asdf

@RX14
Copy link
Contributor

RX14 commented Dec 7, 2016

@kostya If you have to do that, it's a sign that you should split the expression into multiple variables.

Rational:
  To facilitate chaining of method calls without successive try
  wrappers, the addition of a default value to the try call that is
  returned when the receiver is nil allows the returned type to be made
  homogeneous.

  ```
  my_string_or_nil.try(&.size)    # => could be Nil or Int32
  my_string_or_nil.try(0, &.size) # => will always be Int32
  ```
  While this is similar to `(my_string_or_nil.try(&.size) || 0)`, the
  use of a default in try more elegantly solves the case when the result
  could be false or nil.
@drosehn
Copy link

drosehn commented Dec 8, 2016

As a newbie to crystal, I'm wondering why this option would be done as:

my_string_or_nil.try(0, &.size) # => will always be Int32

instead of:

my_string_or_nil.try(&.size, 0) # => will always be Int32

(I have no opinion about which would be better, I'm just wondering which one is "more crystal-ish")

@asterite
Copy link
Member

asterite commented Dec 8, 2016

@drosehn The block argument must always come last in the call

@drosehn
Copy link

drosehn commented Dec 8, 2016

Ah, yes, okay. I understand it better now.

@drosehn
Copy link

drosehn commented Dec 8, 2016

Let me use a different example, to see if I understand @asterite 's earlier point. Given:

my_craZclass_or_nil.try(CrazyClass.new, &.size).craZ_meth

this statement will always create a new CrazyClass object, even when my_craZclass_or_nil is not nil.

@asterite
Copy link
Member

asterite commented Dec 8, 2016

@drosehn Exactly!

@dennisjbell
Copy link
Author

@drosehn, The point @asterite makes is valid, though your example makes incorrect use of the proposed try change: the default value is an output value of the method if the receiver is nil, not an input value to the block if the receiver is nil. In your example, the default would likely be -1 or 0, not a computed value.

The intent for this addition is in the spirit of the 80/20 rule where the default is already pre-computed or a simple literal and better handles the case where the receiver might be falsy or the output of the block is falsy. As @bcardiff points out, this has a similar benefit and limitation to using the default on Hash#fetch.

For where the default would need to be compute or memory intensive, then it is recommended to use the ... || compute_the_alternative method, or get the result and wrap it in an if result.nil?.

@drosehn
Copy link

drosehn commented Dec 8, 2016

Oops, my mistake. I missed one change when I copied the original example to make my new one. What I meant was something more like:

my_craZclass_or_nil.try(CrazyClass.new, &.craZ_dup).craZ_meth

I'm not saying I have any good reason to type that specific statement, but right now I'm too tired to think of a more sensible example. I liked the idea when looking at the example of my_string_or_nil.try(0, &.size), but so far I'm running into "or_nil" situations where I need to create a new object of my own crazy classes, so I'm not sure this would be as useful as I first expected it to be.

Disclaimer: I haven't written all that much crystal, so I know I don't have enough experience to rate the feature. I'm asking questions here just to make sure I understand what people are saying. (and I'm typing in this reply just to make sure the fixed version of my example does make sense!)

@miketheman
Copy link
Contributor

#codetriage

Reading through, it seems like there's a couple of opinions expressed here.

@dennisjbell wants the output of .try to be able to be chained to the next method, echoed by @kostya , and this is performed by a default (the change herein). @bcardiff has requested some changes, that appear to be completed.
There's also a bit of merge conflict that would need to be sorted out.

@asterite and @RX14 are in support of using existing methods such as (my_string_or_nil.try(&.size) || 0) is the current best way to accomplish this goal.

How do we drive this discussion to a conclusion? Should .try have a default result?

@RX14
Copy link
Contributor

RX14 commented May 11, 2017

In my biased opinion, this issue has been discussed to completion and can be closed.

@ysbaddaden
Copy link
Contributor

Closing. Above discussions made it clear that .try(&block) || default is a better solution.

@ysbaddaden ysbaddaden closed this May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants