Skip to content

Conversation

@astorije
Copy link
Contributor

See http://ruby-journal.com/becareful-with-space-in-lambda-hash-rocket-syntax-between-ruby-1-dot-9-and-2-dot-0/ for more info.

Extracted from #306 which will be easier to maintain without that commit.

Note that this is also how http://www.rubydoc.info/gems/rubocop/RuboCop/Cop/Style/Lambda defines lambdas like this as well.

@rmosolgo
Copy link
Owner

I'm ok with this, but I think we should add some kind of safeguard to prevent these from slipping back in as files are changed. Otherwise, I'll forget, and start adding more -> (...! For example, I currently have a branch of documentation rewrites, and I'm sure it's full of that.

What do you think about adding something to the unit tests that makes sure this rule is followed? It would be ok with me to do something like:

it "uses 1.9.3-compatible stabby literals" do 
  invalid_stabby_pattern = /->\s+\(/
  assert_project_files_dont_match(invalid_stabby_pattern)
end 

(Implementing assert_project_files_dont_match is a TODO :P )

Or, if you have another suggestion, I'm open to something else! I just don't trust myself to remember this style.

@rmosolgo
Copy link
Owner

Alternatively, is this transform predictable enough to be maintained as a git hook? I mean, if it's a "simple" find and replace, maybe it wouldn't be so bad to maintain it on the 1.9.3 branch. Just a thought, I'm not a git hook user so I can't really take it farther than that :P

@astorije astorije force-pushed the astorije/lambda-defs branch from 13fc4ed to 0b61309 Compare October 16, 2016 01:51
@astorije
Copy link
Contributor Author

I think we should add some kind of safeguard to prevent these from slipping back in as files are changed.

Fine by me.

Otherwise, I'll forget, and start adding more -> (...! For example, I currently have a branch of documentation rewrites, and I'm sure it's full of that.

Well, until we enforce that, me rebasing the 1.9.3 branch on master and letting the almighty tests give their judgment would be a good enough non-permanent solution IMO...

What do you think about adding something to the unit tests that makes sure this rule is followed?

I could do that as a fallback solution, but I don't want to cloud your test suite with something like that.

Or, if you have another suggestion, I'm open to something else! I just don't trust myself to remember this style.

I looked a bit, and it seems to me that 99% of the web uses ->(. This is why I opened rubocop/rubocop#3631. I'll see what the maintainers say on this and maybe we could merge this and pay attention manually until there is a cop for it that we can enforce here?
Sure, it wouldn't fix documentations but I think that would be the cleanest way to do so, the rest would come naturally.

I mean, if it's a "simple" find and replace, maybe it wouldn't be so bad to maintain it on the 1.9.3 branch.

This doesn't seem very idiomatic in Ruby so I'd rather fix it upstream. Plus I don't know if this is compatible with all implementations of Ruby, such as JRuby.

is this transform predictable enough to be maintained as a git hook?

Probably, but git hooks are not server side, and they aren't added to the repo, so any contributor would need to have the hook, definitely not something we can do.

Let's wait and see for what the Rubocop guys have to say about this. In the meantime, I am happy to periodically check for this should you decide to merge this without enforcing it automatically.

@astorije
Copy link
Contributor Author

@rmosolgo, author of RuboCop and the Ruby style guide agreed keeping the style without space is better on rubocop/rubocop#3631. Mind merging this then?

If no one is creating a cop for this, I'll look into how to do that myself when I have a bit of time.

@rmosolgo
Copy link
Owner

👍 cool, I'm happy to just do my best for the time being! I guess I picked up habits for -> from coffeescript and elixir, where it's usually padded with spaces.

@rmosolgo rmosolgo merged commit 2e59be6 into rmosolgo:master Oct 17, 2016
@astorije astorije deleted the astorije/lambda-defs branch October 17, 2016 17:31
cjoudrey added a commit to cjoudrey/graphql-ruby that referenced this pull request Oct 17, 2016
@rmosolgo rmosolgo modified the milestone: 0.19.4 Oct 18, 2016
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.

2 participants