hydrogen-python icon indicating copy to clipboard operation
hydrogen-python copied to clipboard

fix null reference on empty cell. fixes #14

Open phromo opened this issue 7 years ago • 5 comments

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 avatar Oct 31 '18 08:10 phromo

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

phromo avatar Oct 31 '18 08:10 phromo

For reference, the latest Atom is 1.32.1

kylebarron avatar Oct 31 '18 14:10 kylebarron

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?

nikitakit avatar Nov 04 '18 01:11 nikitakit

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

phromo avatar Nov 04 '18 17:11 phromo

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

kylebarron avatar Nov 04 '18 17:11 kylebarron