search

nikitakit / hydrogen-python

Python-specific plugin for hydrogen. Make Python coding in the Atom editor even more interactive!
https://atom.io/packages/hydrogen-python
MIT License
54 stars 20 forks source link

fix null reference on empty cell. fixes #14 #21

Open phromo opened 5 years ago

phromo commented 5 years ago

if the file contains a trailing code block marker (# %%) there will be an empty cell passed to execute with code null. This seems to be the expected behaviour from hydrogen since it explicitly adds a cell ending at file ending.

phromo commented 5 years ago

I've verified this fixes the issue with no other bugs using the latest atom (1.30.0)

kylebarron commented 5 years ago

For reference, the latest Atom is 1.32.1

nikitakit commented 5 years ago

Thank you for looking into this!

I can reproduce the issue when there is a trailing code cell separator on the last line of the file.

Having code set to null looks to me like a bug in Hydrogen itself. I'm on Atom 1.29 right now, and even if I completely disable hydrogen-python I still get an error thrown from inside Hydrogen:

TypeError: Cannot read property 'match' of undefined
    at TextEditor.isBufferRowCommented (/Applications/Atom.app/Contents/Resources/app/src/text-editor.js:3872:61)
    at isBlank (......./hydrogen/lib/code-manager.js:64:55)

Might it be more appropriate to patch Hydrogen instead?

phromo commented 5 years ago

I had the same line of thought as you but got no hydrogen error when disabling (1.30). I guess creating an issue at hydrogen might be worth a shot - but easier to see the consequences in your plugin. For hydrogen I'm unsure of my standpoint. I think having empty cells is something that would be needed anyway since the user can produce them.. The question for hydrogen is whether empty cells should be left as is (null), filtered (deleted) or changed to the empty string. Deleting cells feels like an extreme that might break other assumptions when it comes to drawing (ie showing the evaluated checkmark). So imho it should be either null or empty string, and since it is currently null.. Why change it

kylebarron commented 5 years ago

@nikitakit It would probably be good to post an issue about that on Hydrogen.