-
-
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
Allow block after bracket call syntax #14326
base: master
Are you sure you want to change the base?
Allow block after bracket call syntax #14326
Conversation
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.
LGTM
This PR does not actually add support for |
If by "add support" you mean parsing it's already parsed correctly, and works perfectly fine, and was working perfectly fine prior to this PR. The problem is with the formatter. Some of my code in particular uses I tried looking into it, but the formatter code is too... complicated for me as an outsider, I guess. What I saw (or what I believe I saw) is that Anyway. Sorry for the long response, I couldn't resist :) |
It's also possible that Though in Ruby this syntax works |
OK let's clarify this:
|
Gosh, it removes it?! Okay. Let's see. This is getting more complicated than I thought :) As it always does...
|
I think this PR is good. We'll need to bring the formatter along to support the syntax. But that shouldn't be hard. I can take a look at it. @oprypin Why do you think the short block syntax Anyway, it's probably OK to leave the short block syntax for a follow-up. If we need more discussion about whether it should be supported, we can do that in a dedicated issue. |
Okay, so I looked into the code. The fix for I did succeed in fixing the formatter issue, mostly. By refactoring the body of this condition into a separate method, let's say foo(
1,
# 2,
# 3,
&.foo
) ... fails in a very very ugly way: foo(
1 # 2,
# 3,
,
&.foo
) Additionally the following: foo(
# Comment
&.z
) ... formats to this: foo(
# Comment
&.z) I suspect there are more issues like these ones, maybe some of them are already known. And my fix inherits them, causing a few failing tests. I've tested the above in 1.11.2, same kind of failure, so it's not me. Formatting And in the end, I'm lost! 🤣 |
Do you have a branch with your changes available? |
It's working on my projects, I'm already using it and I don't think it broke any one of my files. But formatting bugs begin when indentation and multiline appear. I think I remember writing |
Block arg case in Ruby: class C
def [](&f)
f.call(X.new)
end
def []=(*args, **opts, &f)
f.call(X.new, args, opts)
end
end
class X
def test
puts "OK"
end
def test2(args, opts)
puts "#{args}, #{opts}"
end
end
c = C.new
c[&:test] # "OK"
c[1, 2, x: 3, y: 4, &:test2] = 5 # [1, 2, {:x=>3, :y=>4}, 5], {} This error:
is a semantic one, not a syntactic one, because a captured |
@HertzDevil The formatter also inserts a bad comma whenever one introduces a newline. E.g.
|
Fixes #13975. I've been hitting this issue very much lately, I'm using
self.[]
in many of my projects for "smart constructors" and they sometimes require blocks. There's also a bug in the rough vicinity,foo[&.bar]
andfoo[1, &.bar]
etc. causing the formatter to crash, maybe I'll try to fix it sometime later.