Custom body feature #14

htorrence · 2017-08-24T19:31:08Z

Changes the options attribute to be a dictionary and adds argument parsing to allow for custom notification messages.

Closes issue #13

parsing-science

@htorrence: This looks really good! I'm excited to see this feature added.

I have a few suggestions to clean up the code.

Two other things:

Can you update the README with an example of how to use this?
When I put in a string, the quotation marks are also included in the pop-up message. Can you see if there's a way to avoid that? You might have to strip the quotation marks or perhaps there's some other way.

parsing-science · 2017-08-24T19:36:50Z

jupyternotify/jupyternotify.py

@@ -5,6 +5,7 @@

 from IPython.core.getipython import get_ipython
 from IPython.core.magic import Magics, magics_class, cell_magic
+from IPython.core.magic_arguments import magic_arguments, argument, parse_argstring 


As @mdagost can tell you, I'm super into alphabetization so can you alphabetize these imports please?

And while you're there, my errant non-alphabetization in the line above so that @parsing-science doesn't yell at me? :)

parsing-science · 2017-08-24T19:37:40Z

jupyternotify/jupyternotify.py


+    @magic_arguments()
+    @argument("message", nargs="?", default="Cell Execution Has Finished!!",
+          help="Custom notification message")


I think your line is short enough that you can have this argument on the same line as the other ones.

parsing-science · 2017-08-24T19:38:07Z

jupyternotify/jupyternotify.py

    @cell_magic
    def notify(self, line, cell):
+
+       # custom message


Looks like this comment needs to be over one space.

parsing-science · 2017-08-24T19:55:55Z

jupyternotify/jupyternotify.py

    @cell_magic
    def notify(self, line, cell):
+
+       # custom message
+        args = parse_argstring(self.notify, line)


Did you do this in two separate lines in case we have multiple arguments in the future?

Would this work:

self.options["body"] = parse_argstring(self.notify, line).message

changed - this is just argparse habit on my part, no real reason here

parsing-science · 2017-08-24T19:57:26Z

jupyternotify/jupyternotify.py


+    @magic_arguments()
+    @argument("message", nargs="?", default="Cell Execution Has Finished!!",


I think we probably want to have the arguments be called with dashes as you would in the command line. What do you think?

So something more like:

@argument("-m", "--message", nargs="?", default="Cell Execution Has Finished!!", help="Custom notification message")

Also, what is nargs?

I followed the suggested format from the issue here - it feels a little more natural to me for a magic function to have the call be %%notify "cell finished" than %%notify -m "cell finished", but it's an easy switch if you think command line notation is better.

nargs just specifies that some number of arguments should be gathered into a list, the question mark sets it so that if no argument is present then the default value is used. Argparse has weird notation for optional arguments (unless there's a better way of doing this I don't know about).

I agree that with one argument, not having the -m makes sense. I guess it depends on whether we think we're likely to have more arguments in the future. What do you think @mdagost, @htorrence?

@htorrence: I removed nargs from my code, and it seems to use the default correctly without it.

parsing-science · 2017-08-24T19:59:25Z

jupyternotify/jupyternotify.py

@@ -35,7 +43,7 @@ def notify(self, line, cell):
            jsString = jsFile.read()
        display(Javascript(jsString % {
            "notification_uuid": notification_uuid,
-            "options": self.options,
+            "options": json.dumps(self.options),


What's the purpose of the json.dumps? It seems like it would work even if you didn't do this?

You get a javascript error without it - I don't think the Javascript conversion can handle being given a python dictionary

Hmm, I tested removing that part, and it worked for me. What error were you seeing, @htorrence ?

I'm getting this:

As discussed in person, json.dumps is necessary.

adding custom message note

mdagost · 2017-08-25T13:28:35Z

@htorrence Can you do two more things? Can you update the CHANGELOG and bump the version number in setup.py?

Thanks!

Adds image of custom message and updates syntax for custom message call

…upyter-notify into custom_body_feature pulling readme updates locally

parsing-science

Great work @htorrence! Merge away.

parsing-science · 2017-08-25T16:42:33Z

jupyternotify/jupyternotify.py

@@ -35,7 +43,7 @@ def notify(self, line, cell):
            jsString = jsFile.read()
        display(Javascript(jsString % {
            "notification_uuid": notification_uuid,
-            "options": self.options,
+            "options": json.dumps(self.options),


As discussed in person, json.dumps is necessary.

Hanna Torrence added 2 commits August 24, 2017 14:20

adds argument parsing for custom message

66d30ab

style fixes

1fc2f03

htorrence requested review from mdagost and parsing-science August 24, 2017 19:31

parsing-science suggested changes Aug 24, 2017

View reviewed changes

Hanna Torrence and others added 2 commits August 24, 2017 17:13

alphabetization, spacing, and quote removal

7f5ec6f

Update README.md

e166aa1

adding custom message note

htorrence and others added 3 commits August 25, 2017 09:43

Update README.md

53c2cea

Adds image of custom message and updates syntax for custom message call

adds flags to argument, updates changelog and setup.py

5cd0dae

Merge branch 'custom_body_feature' of https://github.com/ShopRunner/j…

49882e7

…upyter-notify into custom_body_feature pulling readme updates locally

parsing-science approved these changes Aug 25, 2017

View reviewed changes

htorrence merged commit 488814d into master Aug 25, 2017

htorrence deleted the custom_body_feature branch August 25, 2017 17:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Custom body feature #14

Custom body feature #14


		@magic_arguments()
		@argument("message", nargs="?", default="Cell Execution Has Finished!!",

Custom body feature #14

Custom body feature #14

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment