Comments#take_before fails with postfix if

[7even]

Here's the minimal example I managed to assemble to show the problem:

require 'parser/current'
require 'unparser'

ast, comments = Parser::CurrentRuby.parse_with_comments(<<-RUBY)
  do_something if condition
  # comment
RUBY

Unparser.unparse(ast, comments) # NoMethodError: undefined method `begin_pos' for nil:NilClass

The problem is here: nil is assigned to range, and then we have nil.begin_pos.

[mbj]

@7even I should have tested the comment PR more strictly. This is the second crashing bug in this code. Gonna rewrite it.

[misfo]

@mbj Before I let you pass the buck to me I figured I'd do a test:

$ git checkout 0a7401e9460dfe4e28476e39864b3f7ab2a59538 # the last commit in my PR
$ ruby -Ilib test-20.rb 
if condition
  do_something
end
# comment
$ git checkout master
$ ruby -Ilib test-20.rb 
~/unparser/lib/unparser/comments.rb:84:in `block in take_before': undefined method `begin_pos' for nil:NilClass (NoMethodError)
    #...

So the bug above was introduced after the my PR was merged.

Blaming bugs on volunteers doesn't seem to be a great way them to attract them to your projects...

[mbj]

@misfo I'm talking about the PR and what was done to it. NOT the persons!!! I refactored it because of my preferred code style, this introduced the bug. Its totally my fault I did not tested it enough. Sorry for misunderstanding here. I'm not a native speaker! (But this case was missing a test ;) )

Normally I run unparser against a ton of ruby libraries to ensure it does not crash, I did not followed this procedure here.

As long unparser is not mutation covered we sadly have to live with this kind of things.

[dkubb]

FWIW, it'd probably be frowned upon for someone to call out another person's contribution as being "bad". I hate it when blame is assigned to someone.. if something fails it's because there's a bug in the process. I didn't see @mbj was blaming one specific person by name.

All code has bugs, and all code can be improved. Also new constraints can invalidate perfectly working code. If a PR gets merged into a project that later turns out to have bugs then we need to look at the process that resulted in it and fix that. For the most part the people who are watching and contributing to ROM related projects are top notch, and given that we have a pretty high bar the quality of contributions we do receive are pretty awesome.

In the case of unparser (and mutant) we know one of the largest weaknesses of the project is that we had to write a bunch of code to reach a point where we could reliably mutation test things. There's tons of code that isn't mutation covered. What I would love to see is a concerted effort to mutation test new code, and then go back and begin backfilling missing tests until we reach a point similar to some of the other projects we all watch.

[mbj]

@dkubb Thx.

@misfo:

To clarify. I wrote "I should have tested that comment PR more strictly". As you authored 99% of the comment PR it looks like I tried to "blame delegate". Me apologize. In my mind the code style refactorings we did AFTER the PR was merged somehow belonged to the PR. Generally I was blaming the process behind that feature.

I typically do explorationary testing, or write my own spike, for all features I accept to my opensource code. Here I just "took and refactored" it without that deep insight. I blamed this process failure, not you in person. I'm gonna rewrite the affected code, refactoring the way the location is used. Mostly to become familiar with the internals of comments in parser. I hope I can find more spots where unit tests are missing.