Skip to content

Conversation

@phromo
Copy link

@phromo phromo commented Oct 31, 2018

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
Copy link
Author

phromo commented Oct 31, 2018

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

@kylebarron
Copy link

For reference, the latest Atom is 1.32.1

@nikitakit
Copy link
Owner

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
Copy link
Author

phromo commented Nov 4, 2018

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
Copy link

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants