Skip to content
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

Display error highlight in application code instead of gem files #54

Closed
wants to merge 1 commit into from

Conversation

karreiro
Copy link
Contributor

Fixes #35

This PR updates the ErrorHighlight.spot method to use the first line that doesn't belong to a Gem file.

Before

After

@karreiro
Copy link
Contributor Author

(I'm taking a look at the prism failure)

@mame
Copy link
Member

mame commented Oct 29, 2024

Thanks, but #35 is a really harder problem than I first thought.

There are two sub issues in #35: (1) which stack frame to display snippets for, and (2) how to display error messages.

For (1), this PR checks the file path of each frame to skip gem files unconditionally. TBH, I don't like this approach. Since the Ruby interpreter does not have a clear distinction between gem code and app code, I am not comfortable with error_highlight making such a distinction on its own. Also, I have no certainty if it is really useful for the application author.

In #35, I wrote error_highlight_skip_frames: 1 to make it explicit how many frames to be skipped, but this was a very bad idea. Kernel#warn already has such an argument, but it is now known to be very difficult to treat.

So, now I don't have a good idea how to go about #35. Should we provide a configurable API for which frames to be skipped? But I am afraid that such an API will be regretted later.

For (2), this PR does nothing. However, I find the following error message quite confusing:

$ cat test.rb
require "json"
def foo
  JSON.parse(nil)
end
foo

$ ruby test.rb
/home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON::Ext::Parser#initialize': no implicit conversion of nil into String (TypeError)

  JSON.parse(nil)
             ^^^
        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'Class#new'
        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON.parse'
        from test.rb:3:in 'Object#foo'
        from test.rb:5:in '<main>'

Seeing this, few users will understand that the snippet is in test.rb:2.

I wonder if we should put the snippet after from test:2:in '<main>', as follows.

$ ruby test.rb
/home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON::Ext::Parser#initialize': no implicit conversion of nil into String (TypeError)
        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'Class#new'
        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON.parse'
        from test.rb:3:in 'Object#foo'

  JSON.parse(nil)
             ^^^
        from test.rb:5:in '<main>'

This may be easy to understand, but unfortunately, the current API of Exception#detailed_message does not allow this. If we really want to do this, we have to first propose an appropriate API on the Ruby side.

@karreiro
Copy link
Contributor Author

karreiro commented Nov 3, 2024

Thanks so much for sharing these thoughts, @mame!

Thanks, but #35 is a really harder problem than I first thought.

It’s harder than I thought, too. I’ve been thinking about it over the past few days.

I have no certainty if it is really useful for the application author.

Indeed, we have no certainty on that point—it’s something that’s been echoing in my mind as well.

--

(2) how to display error messages

With that in mind, here’s a thought: what if we showed two frames instead of one? This might provide the author a bit more context:

$ ruby test.rb
/home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON::Ext::Parser#initialize': no implicit conversion of nil into String (TypeError)
    |
220 |      Parser.new(source).parse
    |                 ^^^^^^

test.rb:5
    |
  5 |  JSON.parse(nil)
    |             --- 

        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'Class#new'
        from /home/mame/.rbenv/versions/ruby-dev/lib/ruby/gems/3.4.0+0/gems/json-2.7.2/lib/json/common.rb:220:in 'JSON.parse'
        from test.rb:3:in 'Object#foo'
        from test.rb:5:in '<main>'
  • The first frame could show the root cause of the error, using ^^^ to highlight the key part, as we were doing before
  • The second frame could then show the relevant app code, with --- to link it back to the root cause

This way, someone could look at the root error and still see the relevant piece of the app code.

What do you think about this approach?

(It's not so verbose as Python, and still provides extra clues for the author. Also, personally, I find error traces very helpful for pinpointing the root cause, so I’ve tried to avoid interpolating the stack trace with the snippet)

--

(1) which stack frame to display snippets

I’ve noticed that rubygems overrides Kernel#require with a custom implementation to load gems on demand.

I think using something like Gem.loaded_specs.map(&:last).map(&:full_gem_path) might be a better approach for distinguishing gem code from app code (as compared to Gem.path).

--

Really curious to hear your thoughts about this proposal. I’m happy to evolve it in a different direction or pause this a bit if we're not entirely on board with any of these paths—we can always revisit this in the future as well :)

Thanks again for your thoughts!

@karreiro
Copy link
Contributor Author

karreiro commented Dec 8, 2024

Closing this PR for now, as I don't think we're quite there yet.

@karreiro karreiro closed this Dec 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Display a line in application code instead of gems
2 participants