-
Notifications
You must be signed in to change notification settings - Fork 4
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
Dev/implement magics #35
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #35 +/- ##
==========================================
+ Coverage 54.68% 55.08% +0.40%
==========================================
Files 10 11 +1
Lines 598 688 +90
==========================================
+ Hits 327 379 +52
- Misses 271 309 +38
Continue to review full report at Codecov.
|
This is a first pass effort to implement line and cell magic functions.
c7874e2
to
9134924
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.
I had too google what magics are and the information I found wasn't very good; could you explain what this PR is trying to do, why, and how it does it? Thanks!
@@ -1,6 +1,7 @@ | |||
import jupyter.wire.kernel; | |||
import jupyter.wire.message : CompleteResult; | |||
|
|||
import jupyter.wire.magic: magic_runner; | |||
import std.stdio; |
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.
?
@@ -10,6 +11,17 @@ class ExampleException: Exception { | |||
mixin basicExceptionCtors; | |||
} | |||
|
|||
static this() { |
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.
Did you mean for this to be thread local?
@@ -10,6 +11,17 @@ class ExampleException: Exception { | |||
mixin basicExceptionCtors; | |||
} | |||
|
|||
static this() { | |||
magic_runner.register_line_magic("echo", | |||
function ExecutionResult(string x) { |
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.
Why not just &textResult
?
return textResult(x); | ||
}); | ||
magic_runner.register_cell_magic("echo", | ||
function ExecutionResult(string x, |
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.
(x, y) => textResult(y)
@@ -19,6 +31,10 @@ struct ExampleBackend { | |||
ExecutionResult execute(in string code, scope IoPubMessageSender sender) @safe { | |||
import std.conv: text; | |||
|
|||
auto c = magic_runner.run(code); |
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.
The indentation is off.
@@ -19,6 +31,10 @@ struct ExampleBackend { | |||
ExecutionResult execute(in string code, scope IoPubMessageSender sender) @safe { | |||
import std.conv: text; | |||
|
|||
auto c = magic_runner.run(code); | |||
if (c[0] == 1) { |
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.
magic numbers.
@safe ExecutionResult function(string)[string] line_magic_map; | ||
@safe ExecutionResult function(string, string)[string] cell_magic_map; | ||
Tuple!(int, ExecutionResult) run(string command) @safe { | ||
import std.string; |
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.
Please only import what's needed instead of a whole module.
import std.array : join; | ||
auto line_regex = regex(r"^%([a-zA-Z0-9]+)\s*"); | ||
auto cell_regex = regex(r"^%%([a-zA-Z0-9]+)\s*"); | ||
string[] cell_string_array; |
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.
It's usually not a good idea to encode type in a name. I suggest cellStrings
.
} | ||
auto cell_string = join(cell_string_array, "\n"); | ||
foreach (string i; cell_items) { | ||
auto c = matchFirst(i, regex(r"^%%([a-zA-Z0-9]+)\s*")); |
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.
Why isn't this indented under the foreach?
First pass an implementing magics. Comments welcome.