-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Replace Indexable#at
with #fetch
#6296
Replace Indexable#at
with #fetch
#6296
Conversation
Please open an issue to talk about this, what the community thinks about |
ac293f6
to
e7e4f5f
Compare
Why do you need for an issue for this if there is already PR? It's even available by (UPD: And PRs in search results along with issues: https://github.com/crystal-lang/crystal/search?q=indexable+at&type=Issues Anybody can discuss anything here about code, approach, needing. |
By the way, I like this PR. It makes the API a bit more consistent with |
Huum, I'd be fine with |
Well, an array could be viewed as a mapping from integers to some values, with missing keys being those outside the array's range. For example in PHP arrays are both arrays and hashes (not that it means something, but it's a nice mathematical view). |
Can |
They can, but we try to avoid aliases in Crystal. |
Is there a reason to this syntax be invalid? array[100] { nil } |
It looks a bit weird. And it's not valid Ruby syntax either (maybe for a reason). |
I like the idea. Both For reference: in Java both
|
We could think about renaming |
For me,
Are you about renaming { a: 1, b: 2, c: 3 }.values_at(:c, :a) #=> [3, 1]
{ a: 1, b: 2, c: 3 }.values #=> [1, 2, 3] And |
Note that we could add |
@AlexWayfer No, I meant renaming |
Oh, OK, I understood, thanks. It may be This is incorrect, I think. |
Just for notice: checks fail not because of code. |
This is valid Ruby (2.5) code: def [](idx, &block)
puts idx + block.call
end
self[1] { 2 } |
@hugoabonizio Cool! Let's add that to Crystal too. |
Here's an initial diff, if someone wants to take care of this: diff --git a/spec/compiler/parser/parser_spec.cr b/spec/compiler/parser/parser_spec.cr
index ecdf0db3c..4e52e658d 100644
--- a/spec/compiler/parser/parser_spec.cr
+++ b/spec/compiler/parser/parser_spec.cr
@@ -124,6 +124,10 @@ module Crystal
it_parses "foo[] /2", Call.new(Call.new("foo".call, "[]"), "/", 2.int32)
it_parses "foo[1] /2", Call.new(Call.new("foo".call, "[]", 1.int32), "/", 2.int32)
it_parses "[1] /2", Call.new(([1.int32] of ASTNode).array, "/", 2.int32)
+ it_parses "foo[1] { 2 }", Call.new("foo".call, "[]", args: [1.int32] of ASTNode, block: Block.new([] of Var, 2.int32))
+ it_parses "foo[1]? { 2 }", Call.new("foo".call, "[]?", args: [1.int32] of ASTNode, block: Block.new([] of Var, 2.int32))
+ assert_syntax_error "foo[1, &x] { 2 }", "block already specified with &"
+ assert_syntax_error "foo[1, &x]? { 2 }", "block already specified with &"
it_parses "!1", Not.new(1.int32)
it_parses "- 1", Call.new(1.int32, "-")
diff --git a/spec/std/indexable_spec.cr b/spec/std/indexable_spec.cr
index 344e5bda9..e2110bebb 100644
--- a/spec/std/indexable_spec.cr
+++ b/spec/std/indexable_spec.cr
@@ -114,4 +114,12 @@ describe Indexable do
return_value.should eq(indexable)
last_element.should eq(3)
end
+
+ {% if compare_version(Crystal::VERSION, "2.5.1") > 0 %}
+ it "does [] with block" do
+ indexable = SafeIndexable.new(5)
+ (indexable[1] { 2 }).should eq(1)
+ (indexable[10] { 2 }).should eq(2)
+ end
+ {% end %}
end
diff --git a/src/compiler/crystal/syntax/parser.cr b/src/compiler/crystal/syntax/parser.cr
index 6d2d509f2..6bfef1b81 100644
--- a/src/compiler/crystal/syntax/parser.cr
+++ b/src/compiler/crystal/syntax/parser.cr
@@ -715,13 +715,6 @@ module Crystal
@wants_regex = false
next_token
- if call_args
- args = call_args.args
- block = call_args.block
- block_arg = call_args.block_arg
- named_args = call_args.named_args
- end
-
if @token.type == :"?"
method_name = "[]?"
next_token_skip_space
@@ -730,6 +723,16 @@ module Crystal
skip_space
end
+ if call_args
+ args = call_args.args
+ block = call_args.block
+ block_arg = call_args.block_arg
+ named_args = call_args.named_args
+ end
+
+ curly_block = parse_curly_block(block_arg)
+ block = curly_block if curly_block.is_a?(Block)
+
atomic = Call.new(atomic, method_name, args: (args || [] of ASTNode), block: block, block_arg: block_arg, named_args: named_args, name_column_number: column_number).at(location)
atomic.name_size = 0 if atomic.is_a?(Call)
atomic
diff --git a/src/indexable.cr b/src/indexable.cr
index d5dc7df4a..84149f609 100644
--- a/src/indexable.cr
+++ b/src/indexable.cr
@@ -73,6 +73,18 @@ module Indexable(T)
at(index)
end
+ # Returns the element at the given *index*, if in bounds,
+ # otherwise executes the given block and returns its value.
+ #
+ # ```
+ # a = [:foo, :bar]
+ # a[0] { :baz } # => :foo
+ # a[2] { :baz } # => :baz
+ # ```
+ def [](index : Int)
+ at(index) { yield }
+ end
+
# Returns the element at the given *index*.
#
# Negative indices can be used to start counting from the end of the array. Adding the |
/cc #6324 |
@asterite |
@RX14 I guess that's fine. We would eliminate If someone want to go for it, you can use my diff above as a start :-) |
Doesn't work for falsey values, I guess.
With blocks?
Let's add the ability to define
|
That's where you would use the block variant.
Yes, in Ruby 2.5.1, although it seems they forgot to document it. |
No, they didn't. It's valid for some reason, but doesn't work as expected. |
Having the triplet |
I don't like this idea at first sight.
But IMHO |
|
No we don't have |
01bb216
to
775f2a1
Compare
Conflicts resolved. |
def [](key) | ||
fetch(key) | ||
fetch(key) do | ||
if (block = @block) && key.is_a?(K) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part below seems wrong to me:
if ... key.is_a?(K)
else
raise KeyError.new "Missing hash key: #{key.inspect}"
end
Why wrong key
type should trigger such error?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was so (look below). This code just moved.
And, I guess, because Ruby works like that:
If the key can't be found, there are several options: With no other arguments, it will raise a KeyError exception;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And, I guess, because Ruby works like that:
Ruby doesn't have strong typing, and so this specific edge-case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Sija If the is_a?(K)
check is not there, invoking the block won't compile. I added that check.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@asterite Would that be wrong thing to do ™? is this check is present inside of other Hash
methods?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand what you mean. There's an issue about restricting these methods and it's a PITA to use, so it as never done.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see, it looked odd to me to have two of those guards as a logic for handling the case of missing key, but your explanation makes total sense, thanks!
775f2a1
to
cdc06a1
Compare
cdc06a1
to
ec67e39
Compare
Conflicts resolved again. |
Add more implementations similar to `Hash#fetch`. No we have: * `[key]` (convenience shortcut for `fetch(key) { raise }`) * `[key]?` (convenience shortcut for `fetch(key) { nil }`) * `fetch(key, default)` * `fetch(key) { default }`
ec67e39
to
00f9271
Compare
Sorry, fails fixed. |
Is it possible to add this PR to |
@bcardiff I think this would be nice in 0.27.0 too |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR also removes Hash#fetch
with no block and make Hash#[]
the only way to access without fall back value. I just needed to view the change since the initial description does not highlight this change.
I think there is a missing AlwaysInline
. After that is done we can merge this before 0.27
Co-Authored-By: AlexWayfer <alex.wayfer@gmail.com>
Per crystal-lang/crystal#6296, calls to Slice#unsafe_at need to be replaced with a call to Slice#unsafe_fetch.
Add more implementations similar to
Hash#fetch
Inspired by #6293 (comment)