Base implementation of inspect request impl. #11
Conversation
|
@SylvainCorlay do you have a clue? |
|
Hey there, sorry for not getting back to you earlier. The way to trigger these inspect requests from a JupyterLab UI is to open the "Contextual Help" from the "Help" topbar menu. Then any keystroke will trigger an inspect request when the contextual help is open. |
|
Hi! I have this working locally and will be adding the changes here (as discussed with @vgvassilev via Skype) : 
|
|
Hourray, this ia amazing! Ping @gouarin for awareness. |
|
Do we have a clang-format configuration for this project? If not, shall we use the standard one from llvm? |
|
@SylvainCorlay, @gouarin what is the way to move forward here? |
|
@mvassilev and @ioanaif thank you for your PR. It's a nice feature and it's great that it's working again! I'm really sorry for the delay and I will try to make a review soon. I also need to look at what @SylvainCorlay did in this project. Since I wrote xeus-cling, I haven't spent much time contributing to it. I will try to change that ;). @vgvassilev we have no clang-format for this project for now but I can add one. |
CMakeLists.txt
Outdated
| @@ -171,7 +171,7 @@ macro(xeus_cpp_set_common_options target_name) | |||
| endif () | |||
|
|
|||
| set_target_properties(${target_name} PROPERTIES | |||
| INSTALL_RPATH_USE_LINK_PATH TRUE | |||
There was a problem hiding this comment.
Why changing the RPATH setting?
There was a problem hiding this comment.
(This seems to be somewhat unrelated to the rest of the changes.)
There was a problem hiding this comment.
@ioanaif Can you comment on this?
There was a problem hiding this comment.
I made that change to have the install work on my local machine (macos) and avoid this error:
-- Installing: /Users/ioana/opt/anaconda3/envs/xeus-cpp/libn/libxeus-cpp.0.dylib
error: /Users/ioana/opt/anaconda3/envs/xeus-cpp/bin/install_name_tool: for: /Users/ioana/opt/anaconda3/envs/xeus-cpp/libn/libxeus-cpp.0.0.1.dylib (for architecture x86_64) option "-add_rpath /Users/ioana/opt/anaconda3/envs/xeus-cpp/lib" would duplicate path, file already has LC_RPATH for: /Users/ioana/opt/anaconda3/envs/xeus-cpp/lib
From the CI logs it seems this is not a necessary change for macos and it was just my environment that was not properly set up.
There was a problem hiding this comment.
So in that case I'd propose to revert these changes.
| } | ||
| */ | ||
| clang::QualType QT = E->getType(); | ||
| return QT.getAsString(); |
There was a problem hiding this comment.
Nice! Really happy that we can get away from the typeid approach.
etc/xeus-cpp/tags.d/xtensor.json
Outdated
| @@ -0,0 +1,4 @@ | |||
| { | |||
| "url": "https://xtensor.readthedocs.io/en/latest/", | |||
| "tagfile": "xtensor.tag" | |||
There was a problem hiding this comment.
I suggest we don't install the xtensor tag files as part of this PR, since it should probably be installed by xtensor itself.
| { | ||
| { std::string exp = R"(\w*(?:\:{2}|\<.*\>|\(.*\)|\[.*\])?)"; | ||
| std::regex re(R"((\w*(?:\:{2}|\<.*\>|\(.*\)|\[.*\])?)(\.?)*$)"); | ||
| auto inspect_request = is_inspect_request(code, re); |
There was a problem hiding this comment.
An inspect request is a different type of message, triggered by keystrokes in the editor when the inspector panel is open.
I don't think we should call this function is_inspect_request but more like is_help_magic
There was a problem hiding this comment.
is_help_magic returns bool so what should be the logic here in the cases of true or false?
| auto inspect_request = is_inspect_request(code, re); | ||
| if (inspect_request.first) | ||
| { | ||
| inspect(inspect_request.second[0], kernel_res, *m_interpreter); |
There was a problem hiding this comment.
Happy to discuss this. Maybe we should not rebind into the inspect function here.
There was a problem hiding this comment.
Maybe this would be related to the previous conversation regarding the replacement of is_inspect_request with is_help_magic
|
I left a few comments in the review. Happy to discuss the changes. It seems that the CI failures come from an issue with the header inclusion order. I have come across this in earlier PRs and had to make sure some things were included earlier than other. |
mvassilev
force-pushed
the
InspectRequestImplementation
branch
from
c0d3681
to
213acd2
Compare
This should fix the bot error: expected ')' before 'PRIxPTR' A similar issue is reported in tensorflow/tensorflow#12998
vgvassilev
force-pushed
the
InspectRequestImplementation
branch
4 times, most recently
from
8efe593
to
a17da7e
Compare
vgvassilev
force-pushed
the
InspectRequestImplementation
branch
2 times, most recently
from
47b9b6b
to
0dbf6b4
Compare
vgvassilev
force-pushed
the
InspectRequestImplementation
branch
from
0dbf6b4
to
a12fe36
Compare
|
@SylvainCorlay, @gouarin, this looks mostly okay to me. @SylvainCorlay, can you take a look and answer to the remaining questions? I have a question on my own: Is there a way to avoid putting |
I don't think this file exist otherwise. |
There is a problem here with the inspect_request_impl function not being invoked which prevents handling input like "? std::vector" that is supposed to display documentation to the user.