search

rossant / ipymd

Use the IPython notebook as an interactive Markdown editor
BSD 3-Clause "New" or "Revised" License
483 stars 42 forks source link

adding notebook and cell metadata #62

Closed bollwyvl closed 9 years ago

bollwyvl commented 9 years ago

The first part of #38, this adds round-trip support for cell metadata.

I probably started in the wrong place, and ended up touching too much stuff. But maybe not. Since I think it has to have a new rule in the lexer, it would have been a bit odd to overload that elsewhere.

Obviously still lots to be done, but wanted to get this out there!

rossant commented 9 years ago

looking good so far! thanks!

bollwyvl commented 9 years ago

Thanks! The test setup you built is really nice... Though i confess i had to do some manual round trips to fix little issues.

Quick question? Why the mistune fork (har)? Nothing is too deeply buried in the upstream (grammar, lexer) and since it's already an nbconvert dep...

On 03:10, Tue, Jun 23, 2015 Cyrille Rossant notifications@github.com wrote:

looking good so far! thanks!

— Reply to this email directly or view it on GitHub https://github.com/rossant/ipymd/pull/62#issuecomment-114386618.

rossant commented 9 years ago

IIRC I had to make minor changes in some classes, but I can't remember the specifics. I'd be happy to move to a regular dependency if it's technically feasible without too much work.

rossant commented 9 years ago

@bollwyvl just wondering if it's still a WIP, or if it can be merged as is, as a first step?

lepture commented 9 years ago

@rossant if there is any reason that you can't make mistune a dependency, I'd like to help.

bollwyvl commented 9 years ago

@rossant gah, totally have a draft response from 11 days ago! anyhow, long and short is i just got back to this now.

I added notebook metadata, maybe not very elegantly. kernel info has to be first, and has to follow the ---<stuff>--- while cells use ---<stuff>... to be more YAML-like.

it works for the content manager (yay!), but of course it is injecting the language_info and kernelspec, and I can't yet decide how to break it down in tests. How do you want to handle this? obviously this is how we would support multiple kernels, but it would be surprising to have all that get injected the first time you round trip. once we do !kernel python2 or whatever, we can obviously hide all that stuff, which would be less surprising, but i think it would be good and useful to get this resolved at least somewhat now before adding the whole alias tag scheme.

rossant commented 9 years ago

kernel info has to be first, and has to follow the ------ while cells use ---... to be more YAML-like.

hmm.. is there a reason for this?

btw can you remind me (or send me a link) about the rationale for --- and ... instead of double ---? Is this standard?

so: about notebook metadata and language info. I haven't thought much about that. A few random remarks

bollwyvl commented 9 years ago

kernel info has to be first, and has to follow the ------ while cells use ---... to be more YAML-like.

hmm.. is there a reason for this?

The first-ness and ---/----ness is the front-matter convention borrowed from Jekyll. Github will render it there, and nowhere else.

It also makes sense, because when it comes back over the wire as an ipynb struct, there will be no way to tease it back out into some other structure that couldn't have been modified by the user, so first is probably as good as any... aside from last.

also there can be only one :zap:.

btw can you remind me (or send me a link) about the rationale for --- and ... instead of double ---? Is this standard?

The YAML spec actually demands ---/..., I don't know why jekyll decided to go with /---. We could allow for /---... but then there would be no distinguisher as to whether something is notebook or cell level. but since we're going to be allowing rather a lot of YAML features, it makes sense to me to go with the spec.

as far as I'm concerned, if I'm working on a book (as I am now), I'll always use the same language for all my .md notebooks,

So if a Configurable setting (i.e. default_kernel) in the profile or cmdline matches what the frontend says in the meta, and there is nothing else at all in there...

maybe the notebook metadata should be opt-out by default? Ideally, the round trip should have no effect by default, I think.

...it doesn't get written out. works for me!

good idea to have aliases like !kernel python2 or whatever -- do you know if there's anything standard for this sort of things?

I presume we can interrogate the kernelmanager or something, and get the list of names. otherwise, it will use the same tag mechanism we talked about for cell meta, like !slide.

bollwyvl commented 9 years ago

Still failing, due to how i've temporarily stored notebook meta, but it now strips out either the native kernel or a configurable one (needs doc). Probably pretty close to reviewable... but keeping [wip] because it's late :zzz:

rossant commented 9 years ago

thanks for the explanations again, looks all quite reasonable!

bollwyvl commented 9 years ago

Ok, tests and doc up! Would love some feedback on the choices: be brutal!

rossant commented 9 years ago

Looks good, thanks! I've left some minor comments. I'll play with it a bit later!

rossant commented 9 years ago

btw, the alias ! thingy doesn't seem to be documented? and it's not used in the test examples?

bollwyvl commented 9 years ago

Thanks for the outstanding code review! :facepunch: The insight about the crufty is_notebook and is_meta cleaned a lot of things up.

Now that I think about it, I don't think I hit the doc on default_kernel: that's there to let you decide which kernel (gah, should be kernels, probably) shouldn't be stored in notebook metadata.

btw, the alias ! thingy doesn't seem to be documented? and it's not used in the test examples?

That's not in yet! It's different and weird enough that I wanted to get all this stuff in first before we explore how that should look. It should be entry_pointy, will need a fair amount of doc, might introduce new dependencies (if we need to do the patch thing). So I guess let me know what you think about what's in, and we'll go from there!

bollwyvl commented 9 years ago

Ok, removed the spurious metadata, documented the kernelspec switches... and made sure they were turned back on.

I have to say, when editing the README, it does seem a bit odd that every paragraph gets its own cell. Also the treatment of things-in-lists is a bit weird. It's outside the scope of this issue, but it is surprising to have a single cell broken into many on roundtrip. With meta now in there, cell boundaries become more important...

rossant commented 9 years ago

awesome, thanks @bollwyvl !!

I'm gonna open a new issue for the split problem