search

tmm1 / ripper-tags

fast, accurate ctags generator for ruby source code using Ripper
MIT License
550 stars 43 forks source link

Have the process survive parse errors in individual files #85

Closed gurgeous closed 6 years ago

gurgeous commented 6 years ago

How about this? #83

gurgeous commented 6 years ago

Sure, I'm always open to feedback.

gurgeous commented 6 years ago

I see that travis failed. I wonder if that test broke due to some subtle Ripper versioning issue when I removed lines from erb_template.rb. I will experiment.

gurgeous commented 6 years ago

I think this will do the trick.

gurgeous commented 6 years ago

Third time is the charm... Comments:

gurgeous commented 6 years ago

Sure. One question - do we keep --force? After this change the only thing it will do is change the exit code from 2 to 0 when errors are encountered.

If we decide to kill that functionality we probably need to leave it in for backward compatibility.

On Thu, Mar 29, 2018 at 7:24 AM, Mislav Marohnić notifications@github.com wrote:

@mislav commented on this pull request.

In lib/ripper-tags/data_reader.rb https://github.com/tmm1/ripper-tags/pull/85#discussion_r178072279:

   file_finder.each_file do |file|

begin $stderr.puts "Parsing file #{file}" if options.verbose extractor = tag_extractor(file) rescue => err

  • if options.force
  • $stderr.puts "Error parsing `#{file}': #{err.message}"
  • else

  • print the error

  • $stderr.puts "Error parsing `#{file}': #{err.message}"
  • @error_count += 1

  • Die if we ran without --force and we haven't seen any tags yet.

  • if !options.force && @tag_count == 0

One final thought about this logic: suppose you run ripper-tags on alib/directory which has 2 files in it:good.rbanderb_template.rb`. The first one will generate some tags, but the second one will enounter a parse error. Because there are already some tags, this condition will not halt processing, but just print to stderr and eventually return with exit status 2.

However, let's say that erb_template.rb gets processed before good.rb. Now the processing halts immediately because the first file had a parse error before any tags were able to get generated. The behavior of ripper-tags is suddenly very different depending on the order of input files.

I know that checking the number of tags is something that I've suggested initially, but now I don't think it's such a great idea. Let's pick consistency and simplicity. How about forgiving all parse errors (while outputting them to stderr) regardless of the number of tags being collected at the time of encountering the error? Let me know your thoughts.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/tmm1/ripper-tags/pull/85#pullrequestreview-108068124, or mute the thread https://github.com/notifications/unsubscribe-auth/AAgHc0GpM_7ZUtxL2b8xf--SSjJPi2GLks5tjO6QgaJpZM4S56wc .

mislav commented 6 years ago

We keep --force for backwards compatibility but change its documentation re: exit code.

gurgeous commented 6 years ago

How about this? The only additional change I might suggest would be to update the README to document exit code 2. We can do that later, though. That file probably needs a refresh anyway.

mislav commented 6 years ago

I've pushed some changes. I've opted against exit status 2 for now. The new logic: exit with status 1 if there were as many parse errors as files (i.e. if all files have failed). Otherwise, exit with 0.

If some tool author requests to be able to distinguish between these cases, we can update exit codes. But for now, I vote to keep it simple.

I've also renamed your PR title to something more descriptive and moved the issue reference from the title (where it's invalid and not a real link) to the PR body so it properly links to the corresponding issue. This is something that I suggest you keep in mind for future PRs. 😉

Thank you for all your help and patience! Would you kindly give your review to my updates? 🙇

skarger commented 6 years ago

@gurgeous @mislav Just wanted to chime in to keep this PR from slipping through the cracks, since I'd benefit from the feature being released. Thanks for your work.

gurgeous commented 6 years ago

Oh, sorry about that! I missed that I was supposed to review. Thanks for the nudge. Give me a sec here...

mislav commented 6 years ago

@skarger This is now v0.6.0.