-
Notifications
You must be signed in to change notification settings - Fork 333
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Import Jupyter Notebook files into Atom #1501
Conversation
Can someone help with this last flow error? My understanding of promises and callbacks is only so-so.
|
Thanks for getting this started 🎉 I guess the flow error is because the callback in I can review the PR in detail next week. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is going to be great @kylebarron! Thanks for leading this effort!
I added some tests and fixed a couple things on my fork. If you add me as a collaborator on your fork I can push my changes to this PR. Or, I can pr against your fork if you prefer.
kylebarron/hydrogen@import-notebook...BenRussert:import-notebook
const cellType = cell.cell_type; | ||
const cellMarkerKeyword = cellType === "markdown" ? "markdown" : null; | ||
const cellMarker = getCellMarker(commentStartString, cellMarkerKeyword); | ||
var source = cell.source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For best practice, use let
instead of var
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
lib/main.js
Outdated
@@ -55,6 +55,7 @@ import { | |||
} from "./utils"; | |||
|
|||
import exportNotebook from "./export-notebook"; | |||
import importNotebook from "./import-notebook"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I really like that you implemented this in its own file and imported into main. This improves organization, avoids merge conflicts from unrelated PRs, and makes testing easier to name a few benefits.
Added as contributor |
We can rebase last once we are ready to merge. Try this branch out and see if you can find anything that still needs work. I'll take another look during the week as well to see what's left. |
} | ||
const nb = parseNotebook(data); | ||
if (nb.nbformat < 4) { | ||
atom.notifications.addError("Only notebook version 4 currently supported"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be good to check out how version 3 differs, because supporting reading version 3 would be a nice plus (probably for a future PR)
const cellType = cell.cell_type; | ||
const cellMarkerKeyword = cellType === "markdown" ? "markdown" : null; | ||
const cellMarker = getCellMarker(commentStartString, cellMarkerKeyword); | ||
var source = cell.source; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
spec/import-notebook-spec.js
Outdated
fail(e); | ||
done(); | ||
}); | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't you import this from test-utils.js
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely meant to 😂
I think all your edits make sense, though I haven't tested them in Atom yet. |
I'd be in favor of moving |
I also think it's important to have tests of [import then export] and [export then import] and to make sure that the |
I'd also be happy to add documentation to https://nteract.gitbooks.io/hydrogen/docs/Usage/NotebookFiles.html. (When #1498 was merged, the documentation was updated despite that code not being released yet). |
I added some more documentation and moved |
@BenRussert I wasn't sure if this should be its own PR, since it might need more discussion, but it would also be useful to add the import notebook functionality as an opener for |
04f78e4
to
b655686
Compare
I know nothing about how this works, but the ideal for me would be to have atom open .ipynb files like this, then export them on save. Everyone around me works directly in jupyter, this way I could work with them pretty seamlessly. On a side note, how about having this import/export to/from the rich document format? Even if it only supported importing one language and markdown cells it would be nice to have the choice of which format to work in. |
We currently don't support automatic exporting because of the potential for unintentionally overwriting data.
Do you mean markdown documents? |
Fair enough, I can add a hotkey for export anyway. Yes I mean markdown documents, then the grammar/kernelspec would be used to set the language on the code blocks. |
Well currently when you export a file to a notebook, it brings up the system file selector, so not sure how much time a hotkey would save. Hydrogen has no way to know ahead of time how to name the outputted notebook file.
Markdown documents are more prone to losing some metadata, in particular cell boundaries. It's probably not too difficult to write, but not my focus at the moment. |
@JohnCHarrington |
This PR adds functionality to import Jupyter Notebook files as text files in Atom.
It imports the new file into a new TextEditor. In order to support Markdown blocks, I need to know the correct comment symbol, so I need to find the correct Atom Grammar for the notebook file. I first try to match file extensions between the notebook file's metadata and the file extensions that each Grammar is applied for. But since the file extension is an optional metadata field for the notebook, I also attempt to match on kernelspec name. These two together should work for the vast majority of notebooks.
Given the grammar, I get the source text for code and markdown cells, prepending each line of text in the markdown cell with the
commentStartString
, right trimmed plus a space. I add a cell marker line before each cell, either# %%
or# %% markdown
. It uses\r\n
as the line separator on Windows;\n
otherwise (for newlines created between cells).To do:
Limitations:
Python
Bash:
Javascript:
R:
Closes #1457, ref #1404, ref #75.