SpencerPark / IJava

A Jupyter kernel for executing Java code.
MIT License
1.09k stars 212 forks source link

Missing space in front of percent sign causes strange error message #31

Closed CSchoel closed 5 years ago

CSchoel commented 6 years ago

The kind of error that you only find with the help of programming novices... :laughing:

jshell> 10 %7
$5 ==> 3

This works fine in the JShell, but in an IJava-Notebook it produces the following error message:

percent_error

Kernel version: 1.1.2

SpencerPark commented 6 years ago

Hello @CSchoel, yes the magics have been somewhat of a pain point with the kernel that I'm not sure how to best handle. Magics are just sugar for the call you see in the error message which works nice if the intention is to use magics but really strange if you aren't (as in this case).

My best attempt at trying to make sure the magics aren't used by accident was to require no spaces, be followed by letters or digits, and not work inside string literals but it's not perfect. I think what I might try is that if the magic compiles, assume it was intentional, if the generated code doesn't, then assume it was not meant to be a magic and leave the original. Another option is to switch out from the % and use : which I wish I had done earlier but is now a very big breaking change with all the notebooks using the maven magic...

If you have any suggestions please let me know. As you seem to be using this for teaching I feel you may have the best insight on how to handle this as you see a lot of poorly written code :)

CSchoel commented 5 years ago

Ok, now I understand the error message. So this is a preprocessing steps for the commands starting with % that is triggered by accident.

As far as I can see all the examples in your documentation have the percent sign at the beginning of the line. Is there a good reason to allow these magics at an arbitrary position within a line? If not, you could just require them to be defined on their own line of code and probably keep most of the existing notebooks intact.

This leaves only some very strange edge cases such as the following to be considered.

int x = (
7
%3);

I think it would be safe to ignore those cases, but as an additional safeguard you might be able to inspect the code that follows the percent sign and if it does not fall into any of the defined categories (%maven something, %%loadFromPOM, etc.) default to keeping the code as it is.

SpencerPark commented 5 years ago

Alright I think I've found a good compromise. The pattern is now more strict and matches what you suggested, magics that are on their own line. It also matches a magic used immediately after (with whitespace) an = sign to support assignments.

This second case is not as common but is possible. For example the %jars magic returns all the jars that the glob matched in a list. One could write:

List<String> added = %jars C:/my/jars/*.jar
added

I believe this should cover most cases where magics could be unintentionally used with the one caveat you mentioned putting the %3 on the following line but I think that is uncommon enough to let it slide :)

CSchoel commented 5 years ago

This sound like good tradeoff. I will let you know, if my students are still able to produce this error in the next semester. :laughing:

SpencerPark commented 5 years ago

Now in release 1.2.0.