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

Remove incorrect and bugged inspect_request #119

Merged
merged 2 commits into from
Sep 1, 2019
Merged

Remove incorrect and bugged inspect_request #119

merged 2 commits into from
Sep 1, 2019

Conversation

LunarLanding
Copy link
Contributor

With the previous code, shift+tab on the notebook runs code silently instead of showing the documentation. It's not the intended function of inspect_request and might create confusion with users.
(according to https://jupyter-client.readthedocs.io/en/latest/messaging.html#introspection )

Also, @backend.eval is called with the wrong number of arguments (running on ruby 2.3.0 )

With the previous code, shift+tab on the notebook runs code silently instead of showing the documentation. It's not the intended function of inspect_request and might create confusion with users.
(according to https://jupyter-client.readthedocs.io/en/latest/messaging.html#introspection )
@LunarLanding
Copy link
Contributor Author

Hm, this is incomplete, since according to https://jupyter-client.readthedocs.io/en/latest/messaging.html#compatibility , the kernel should ignore messages for which there is no handler implemented. So more changes are necessary.

@mrkn
Copy link
Contributor

mrkn commented Jun 20, 2017

@LunarLanding Can you continue this pull-request?

@v0dro
Copy link
Member

v0dro commented Jul 3, 2017

@LunarLanding ping!

@LunarLanding
Copy link
Contributor Author

I am continuing it now that I have some free time, sorry for the delay in replying.

@LunarLanding
Copy link
Contributor Author

LunarLanding commented Jul 15, 2017

I have been reading pry's code, specifically the autocomplete section. If we could easily extract the scope of the line being inspected, we could then use almost the same code to obtain the object being inspected, and then it should be relatively easy to display information about it.
In order to extract the scope of potentially incomplete code, I looked into parsers. I started with ripper, as the source code suggested. However incomplete code such as:

class A
    some_method_to_inspect

Makes the parser error and not produce an AST. I looked into https://github.com/whitequark/parser and it seems to have the same problem, although it produces an error message with enough information to know which blocks / parentheses are open. If we dig into the internals we could probably "close" those contexts, get a valid AST, and then get the scope. I'm assuming people use iruby as they did the ipython notebook, i.e. running their definitions first in order to be able to inspect it / autocomplete it in a lower cell, and therefore I am trying to at least get the scope of the inspect request.

@v0dro
Copy link
Member

v0dro commented Aug 3, 2017

@mrkn WDYT?

@mrkn
Copy link
Contributor

mrkn commented Aug 4, 2017

It is great if we can support inspect_request message completely, but it isn't easy as @LunarLanding was investigated.

So I think that deactivating inspect_request message, which is the first aim of this pull-request, is good as the first step to fix it. And then we should make another new issue to realize the ideal inspect_request message handler.

To deactivate it, the simple solution may be making the empty inspect_reply response as below.

diff --git a/lib/iruby/kernel.rb b/lib/iruby/kernel.rb
index b9e9d6e..f7af276 100644
--- a/lib/iruby/kernel.rb
+++ b/lib/iruby/kernel.rb
@@ -142,14 +142,7 @@ module IRuby
     end

     def inspect_request(msg)
-      result = @backend.eval(msg[:content]['code'])
-      @session.send(:reply, :inspect_reply,
-                    status: :ok,
-                    data: Display.display(result),
-                    metadata: {})
-    rescue Exception => e
-      IRuby.logger.warn "Inspection error: #{e.message}\n#{e.backtrace.join("\n")}"
-      @session.send(:reply, :inspect_reply, status: :error)
+      @session.send(:reply, :inspect_reply, status: :ok, found: false, data: {}, metadata: {})
     end

     def comm_open(msg)

@LunarLanding How about this direction?

following @mrkn suggestion.
@v0dro
Copy link
Member

v0dro commented Aug 22, 2017

@mrkn would you like to merge this now?

@kojix2
Copy link
Member

kojix2 commented Sep 1, 2019

I want to merge this pull request.
If the original inspect_request doesn't work, the blank code is better.

@kojix2 kojix2 merged commit 400c955 into SciRuby:master Sep 1, 2019
netbsd-srcmastr pushed a commit to NetBSD/pkgsrc that referenced this pull request May 4, 2021
0.5.0 (2021-03-25)

Bug Fixes:

  * Fix Jupyter console crashes issue
    SciRuby/iruby#210 (@kojix2)
  * Fix syntax highlighting issue on Jpyter Lab
    SciRuby/iruby#224 (@kojix2)
  * Fix interoperability issue with ruby-git
    SciRuby/iruby#139 (@habemus-papadum)
  * Fix the issue of `$stderr.write` that cannot handle multiple arguments
    SciRuby/iruby#206 (@kojix2)
  * Remove a buggy `inspect_request` implementation
    SciRuby/iruby#119 (@LunarLanding)
  * Fix uninitialized constant `Fiddle` caused in initialization phase
    SciRuby/iruby#264 (@MatthewSteen, @kjoix2)
  * Fix the issue on displaying a table
    SciRuby/iruby#281 (@ankane)

Enhancements:

  * Add `IRuby.clear_output` method
    SciRuby/iruby#220 (@kojix2)
  * Make backtrace on exception simplify and more appropriate for code in a
    cell SciRuby/iruby#249 (@zheng-yongping)
  * Make syntax error message more appropriate
    SciRuby/iruby#251 (@zheng-yongping)
  * Remove top-level `In` and `Out` constants
    SciRuby/iruby#229 (@kojix2)
  * Use text/plain for the default format of `Numo::NArray` objects
    SciRuby/iruby#255 (@kojix2)
  * Use ffi-rzmq as the default ZeroMQ adapter
    SciRuby/iruby#256 (@kojix2)
  * Drop rbczmq support SciRuby/iruby#260
    (@rstammer)
  * Add ruby-vips image support SciRuby/iruby#279
    (@ankane)
  * Replace mimemagic with mime-types
    SciRuby/iruby#291 (@mrkn)
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.

4 participants