Parser 2.3 support #801

chastell · 2015-12-28T21:10:32Z

I want to get my pet projects that would benefit from the squiggly heredoc over to Ruby 2.3 ASAP, and for this we need to make sure Reek supports it.

To the best of my quick checks the current blockers are:

(the newest) parser 2.3.0.pre.6 still not supporting the new syntax,
(the newest) unparser 0.4.2 depending on parser '~> 2.2.2'.

Once the above are resolved I want to make sure Reek at least doesn’t blow up on the new syntax, but ideally addresses those parts of the new syntax it should address. ;)

troessner · 2015-12-27T23:14:47Z

I want to get my pet projects that would benefit from the squiggly heredoc over to Ruby 2.3 ASAP

YES, I WANT THAT SQUIGGLY THING TOO!

(the newest) parser 2.3.0.pre.6 still not supporting the new syntax,

We then also need to address the safe navigation operator i guess....

mvz · 2015-12-28T16:33:39Z

Yes! We should support 2.3 syntax ASAP.

Perhaps we can at least start a branch moving to parser 2.3.0.pre.x and adding support for :csend nodes.

I would like to drop unparser and find some other way of printing source (perhaps by just finding it in the original string). The reason is that unparser is still in a below-1.0 version and considered as such by the author, plus we only print tiny bits of code anyway.

troessner · 2015-12-28T16:41:16Z

Perhaps we can at least start a branch moving to parser 2.3.0.pre.x and adding support for :csend nodes.

Good idea! I'm a swamped right now, would anybody like to tackle this problem?

chastell · 2015-12-28T20:34:46Z

I got pack from four days of meeting families and promptly got a 39°C fever (→ staying offline makes me sick), but I’ll get on this one. :)

chastell · 2015-12-28T21:11:48Z

Oooh, the lovely issue → PR transformation API still works. ;)

troessner · 2016-01-14T21:23:29Z

Aaaaaand parser 2.3 is out! One problem though:

  ( $ (master)) bundle update
Fetching gem metadata from https://rubygems.org/.........
Fetching version metadata from https://rubygems.org/..
Resolving dependencies...
Bundler could not find compatible versions for gem "parser":
  In Gemfile:
    reek (>= 0) ruby depends on
      parser (~> 2.3) ruby

    rubocop (~> 0.36.0) ruby depends on
      parser (< 3.0, >= 2.3.0.0) ruby

    reek (>= 0) ruby depends on
      unparser (~> 0.2.4) ruby depends on
        parser (~> 2.2.2) ruby

and that is the latest unparser version. Seems like we need to get #815 going first.

troessner · 2016-01-15T12:51:55Z

I'll start with #815 right away.
@chastell do you have time to handle this issue or are you still sick? If yes, I can start to deal with this.
Sorry for being pushy, but I think we should act quick here. Ruby 2.3 is out for a while now and I believe we have a high probability to alienate our users.
Let's get this going.

mvz · 2016-01-15T13:38:11Z

I'll start with #815 right away.

You're on a roll! Awesome 👍

chastell · 2016-01-15T23:18:41Z

I’m sick again (as in, third time in a row), but I’ll be on this this weekend. :)

(And I agree we should get it fixed ASAP, I have projects I want to switch to the squiggly heredoc!) ;)

chastell · 2016-01-16T01:46:47Z

This is green now and (patiently!) waits for mbj/unparser#60.

troessner · 2016-01-16T10:14:10Z

features/command_line_interface/stdin.feature

@@ -33,7 +33,7 @@ Feature: Reek reads from $stdin when no files are given
      """

  Scenario: syntax error causes the source to be ignored
-    When I pass "def incomplete" to reek
+    When I pass "=" to reek


Why this change?

Parser 2.3.0.1 blows up on parsing def incomplete:

lib/parser/diagnostic.rb:121:in `[]=': 1...1 out of range (RangeError)

I tried to make Parser raise the expected Parser::SyntaxError and = was the first thing that worked. I can try coming up with something less crypting that doesn’t make Parser blow up (but raise what we expect).

(We should probably guard Reek users from Parser blowing up, but that’s a task for another PR…)

Perhaps this should be reported as a bug in parser?

troessner · 2016-01-16T10:46:15Z

This is green now and (patiently!) waits for mbj/unparser#60.

Are those really the only changes we need to do?
I mean

Parser::CurrentRuby.parse("user.&profile")

looks vastly different from

Parser::CurrentRuby.parse("user.profile")

....

chastell · 2016-01-16T20:04:04Z

Are those really the only changes we need to do?

You’re right, these are by far not the only changes we need to do to support the full Ruby 2.3 syntax; this is just the base to actually make Reek being able to depend on Parser 2.3. Still, I’d rather we get this in ASAP (probably by getting #827 in first) so that people can at least upgrade their RuboCops.

I’ll work on supporting what Parser calls on the new syntax ASAP, unless you’d rather I worked on #827 first. :)

troessner · 2016-01-16T21:54:22Z

I’ll work on supporting what Parser calls on the new syntax ASAP, unless you’d rather I worked on #827 first. :)

No, please go ahead by all means. I'm afraid removing unparser is more effort than I had thought. Right now I'm thinking more about actually fixing this in unparser, but let's see).

chastell · 2016-01-17T20:43:27Z

I'm thinking more about actually fixing this in unparser

I did fix the dependency issue in unparser, but if you read that PR you’ll see that it’s unlikely to be merged soon, and I doubt unparser will support the new Ruby 2.3 syntax anytime soon either.

I think we should push on with #827; we can’t release Reek with a runtime dependency on a GitHub repo. Let me know if I can help with #827 – I really want a Reek version that does not block Parser upgrade ASAP (even before we fully support Ruby 2.3), as people currently can’t upgrade to the newest RuboCop if they happen to use Reek as well (and I really don’t want them to stop using Reek).

mvz · 2016-01-17T20:55:44Z

I think we should push on with #827;

I agree. I can help with that one as well if needed.

troessner · 2016-01-18T00:05:12Z

I agree. I can help with that one as well if needed.

That would be highly welcome! #827 is really just a super-early wip branch, nothing more. If you'd like to take over, I'd just close this one. Would you like to tackle this?

mvz · 2016-01-18T07:42:30Z

I can just take over the branch from #827. It seems to at least be heading in the right direction.

troessner · 2016-01-18T08:20:23Z

Ok, sounds great, then I'll just leave it like that ;)

troessner · 2016-01-20T19:21:40Z

Now that #827 is merged lets rebase this one so we can make Reek 2.3 compatible!

troessner · 2016-01-21T21:08:14Z

@chastell let's get this going, you just need to remove this line and then we can merge this one ;)

chastell · 2016-01-21T21:59:54Z

Yes, sorry! This week totally blew up for me. On it!

chastell · 2016-01-21T22:10:59Z

@troessner I limited this PR (and issue) to relaxing our dependency on Parser so that Reek can be installed alonside Parser 2.3. I also made the invalid syntax feature a bit more self-explanatory.

I think we should get this merged and released, and I’ll work on actual Ruby 2.3 support (handling what Parser emits for the new syntax) in a separate PR.

troessner · 2016-01-21T22:40:37Z

I limited this PR (and issue) to relaxing our dependency on Parser so that Reek can be installed alonside Parser 2.3

I dont get it, why not

'parser', ~> '2.3'

right away? Whats the benefit? Parser is fully backwards compatible so i'd rather update now...

chastell · 2016-01-21T22:47:11Z

My reasons for allowing Parser 2.2.2.5 and 2.2.2.6 are:

Reek does work with these versions and I don’t think it will stop working with them anytime soon,
if there is another tool that depends on unparser or Parser < 2.3 then it won’t be usable alongside Reek 3.8.4+ (just as Reek 3.8.3 is not installable alongside current RuboCop),
I’m reluctant to tighten runtime dependencies unless necessary, and I’d say such tightening requires at least a minor version bump.

But if you’re feeling strong about '~> 2.3' then I’ll do it right away so we can release ASAP. :)

troessner · 2016-01-21T23:01:16Z

But that way Reek + ruby 2.3 might work for a user depending on randomness. That is, if he has parser 2.2.2 already installed this doesnt work. On fresh install it would work. Which is really really weird.

Reek does work with these versions and I don’t think it will stop working with them anytime soon,

Conjecture. Maybe, maybe not. Donald Knuth is screaming "premature optimization" in my ear as we speak.

if there is another tool that depends on unparser or Parser < 2.3 then it won’t be usable alongside Reek 3.8.4+ (just as Reek 3.8.3 is not installable alongside current RuboCop),

He is still screaming.

And my closing argument: This is not enterprise java beans but ruby. I believe we should rather take swift and bold actions and not live the live of a "make it right for every user on this planet" slave.

Wdyt?

troessner · 2016-01-21T23:04:36Z

Addendum: i see the rubocop problem but do we really want to limit Reek to some other tool, no matter how popular it is?

chastell · 2016-01-21T23:07:27Z

But that way Reek + Ruby 2.3 might work for a user depending on randomness.

Ok, this is a good point. I bumped this to '~> 2.3'.

I see the RuboCop problem but do we really want to limit Reek to some other tool, no matter how popular it is?

I’m not sure I got my point across: with '~> 2.3' Reek is behaving towards other tools like RuboCop 0.36 behaved towards Reek: if Tool X depends on Parser < 2.3 or depends on unparser then it won’t be installable along current version of Reek.

Meanwhile, this built. ;)

Parser 2.3 support

troessner · 2016-01-22T08:10:44Z

Merged! Release incoming!

troessner · 2016-01-22T08:14:54Z

: if Tool X depends on Parser < 2.3 or depends on unparser then it won’t be installable along current version of Reek.

I see your point, but i beliebe being tentative about this makes dependency hell even worse. We should update aggressively and tool x then just has to update as well.

chastell · 2016-01-22T13:03:37Z

and tool x then just has to update as well

I KNOW, RIGHT? 😆

Thanks so much for the merge!

chastell self-assigned this Dec 27, 2015

chastell added the work in progress label Dec 28, 2015

troessner mentioned this pull request Jan 5, 2016

Get rid of the unparser gem #815

Closed

troessner mentioned this pull request Jan 14, 2016

parser 2.3 is out mbj/unparser#59

Closed

chastell force-pushed the ruby_2.3_support branch from f4233a8 to d9e0a7f Compare January 16, 2016 01:28

troessner reviewed Jan 16, 2016
View reviewed changes

nijikon mentioned this pull request Jan 17, 2016

Rubocop 0.37.2 polishgeeks/polishgeeks-dev-tools#44

Closed

3 tasks

chastell force-pushed the ruby_2.3_support branch 2 times, most recently from c4b06d8 to 77dfc7e Compare January 21, 2016 22:05

chastell changed the title ~~Ruby 2.3 support~~ Parser 2.3 support Jan 21, 2016

chastell force-pushed the ruby_2.3_support branch from 77dfc7e to 15ba789 Compare January 21, 2016 22:45

chastell removed the work in progress label Jan 21, 2016

Bump Parser dependency to support Ruby 2.3

6a58140

chastell force-pushed the ruby_2.3_support branch from 15ba789 to 6a58140 Compare January 21, 2016 23:04

troessner pushed a commit that referenced this pull request Jan 22, 2016

Merge pull request #801 from troessner/ruby_2.3_support

5dd73b3

Parser 2.3 support

troessner merged commit 5dd73b3 into master Jan 22, 2016

troessner deleted the ruby_2.3_support branch January 22, 2016 08:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parser 2.3 support #801

Parser 2.3 support #801

chastell commented Dec 28, 2015

troessner commented Dec 27, 2015

mvz commented Dec 28, 2015

troessner commented Dec 28, 2015

chastell commented Dec 28, 2015

chastell commented Dec 28, 2015

troessner commented Jan 14, 2016

troessner commented Jan 15, 2016

mvz commented Jan 15, 2016

chastell commented Jan 15, 2016

chastell commented Jan 16, 2016

troessner Jan 16, 2016

chastell Jan 16, 2016

mvz Jan 17, 2016

troessner commented Jan 16, 2016

chastell commented Jan 16, 2016

troessner commented Jan 16, 2016

chastell commented Jan 17, 2016

mvz commented Jan 17, 2016

troessner commented Jan 18, 2016

mvz commented Jan 18, 2016

troessner commented Jan 18, 2016

troessner commented Jan 20, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 21, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 22, 2016

troessner commented Jan 22, 2016

chastell commented Jan 22, 2016

Parser 2.3 support #801

Parser 2.3 support #801

Conversation

chastell commented Dec 28, 2015

troessner commented Dec 27, 2015

mvz commented Dec 28, 2015

troessner commented Dec 28, 2015

chastell commented Dec 28, 2015

chastell commented Dec 28, 2015

troessner commented Jan 14, 2016

troessner commented Jan 15, 2016

mvz commented Jan 15, 2016

chastell commented Jan 15, 2016

chastell commented Jan 16, 2016

troessner Jan 16, 2016

Choose a reason for hiding this comment

chastell Jan 16, 2016

Choose a reason for hiding this comment

mvz Jan 17, 2016

Choose a reason for hiding this comment

troessner commented Jan 16, 2016

chastell commented Jan 16, 2016

troessner commented Jan 16, 2016

chastell commented Jan 17, 2016

mvz commented Jan 17, 2016

troessner commented Jan 18, 2016

mvz commented Jan 18, 2016

troessner commented Jan 18, 2016

troessner commented Jan 20, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 21, 2016

troessner commented Jan 21, 2016

chastell commented Jan 21, 2016

troessner commented Jan 22, 2016

troessner commented Jan 22, 2016

chastell commented Jan 22, 2016