-
Notifications
You must be signed in to change notification settings - Fork 28
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
xassist implementation #151
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 15. Check the log or trigger a new build to see more.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #151 +/- ##
==========================================
- Coverage 82.24% 81.97% -0.27%
==========================================
Files 17 19 +2
Lines 625 760 +135
Branches 61 79 +18
==========================================
+ Hits 514 623 +109
- Misses 111 137 +26
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
6849775
to
bc36b14
Compare
cea8bb0
to
9b9378b
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
I think this shows we need an extension system for magics, so that we don't keep to much in core xeus-cpp and have such features in extensions. |
Perhaps we need to implement #4. @tharun571, can you take a look? I guess here the question is should we accept the PR and move it as a plugin or wait for the plugin system? |
d8fe129
to
58a77ab
Compare
I think we can accept the PR and move it to the plugin system when we have it, because the implementation does not spread across the whole kernel. My only concern is the dependency on curl, but we can probably hide it behind a building flag if we package a new version of xeus-cpp before the plugin system is available. Nitpicking: the naming convention of xeus-cpp is snake case, not camel case, but this can be changed later (as a mechanical change that would be fast to review). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
b96a118
to
b24d04a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
405b97e
to
5da9116
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, the failures are actually regression in the master (see #155)
I guess this would involve having a wasm build for curl (to support the esmcripten CI) ? |
Raised a recipe here (emscripten-forge/recipes#1332) I guess having let me know what all files out of the curl library are required here. |
Yes |
I've added the recipe. Just needs to be reviewed by the folks there but in the meantime, any reviews from here (just confirming if I generate everything required or not would be great). Currently I generate
|
Description
Type of change
Please tick all options which are relevant.