pietroppeter / nimibook

A port of mdbook to nim(ib)
https://pietroppeter.github.io/nimibook/
MIT License
74 stars 8 forks source link

Allow embedding a nim file as an nbCode block #85

Open PhilippMDoerner opened 9 months ago

PhilippMDoerner commented 9 months ago

This came to me while writing on the owlkettle nimibook.

Owlkettle has a bunch of example.nim files for example applications for individual widgets. I would love to be able to embed those inside of the owlkettle nimibook instead of linking to the files.

The idea would be something like:

import nimib, nimibook

nbInit(theme = useNimibook)

nbText: """
## Example
Look at this very cool example of a scale widget
"""

nbCode(file = "owlkettle/examples/widget/scale.nim")

nbText: """
As you can see it does wonderful things etc. etc.
"""
...

This is just an example to demonstrate what I mean, what the Syntax for it looks like exactly isn't all that important.

HugoGranstrom commented 9 months ago

Would you like to just show the code or to run it as well?

PhilippMDoerner commented 9 months ago

For my specific usecase I only need to show the code, as another nimble command will compile all the individual examples.

For the feature generally I think it makes sense to compile and run these files same as normal nbCode blocks. An nbCode block gives me the promise normally that the code inside it can definitely compile and run. That is a promise that should be upheld by default imo, anything else breaks what I expect off of an nbCode block.

Could be of course nice if I could pass a boolean that allows me to say "don't compile the file, just show" or enable passing specific compilation flags for the compilation of this file specifically, but I don't think that's particularly necessary for a first stab at the feature.

HugoGranstrom commented 9 months ago

Just showing the file should be easy, but running it as well sounds a bit harder, especially if we want to capture the output of the external program. This would be a separate thing from nbCode though, as this is a totally different thing. Maybe nbCodeFile or nbReadFile or nbRunFile or something else. nbFile already does something similar, but it also writes a file, which isn't what we want in this case.

It should be totally possible, though, it's just a matter of deciding on the details of how it should work. It should be implemented in nimib itself though.

PhilippMDoerner commented 9 months ago

Just in case you want feedback on that nbNimFile and nbReadFile seem sensible:

Though the other options also look perfectly fine. How is this to progress from here? Await feedback from pietroppeter and based on that somebody (me? you? peter?) opens up an issue in nimib?

HugoGranstrom commented 9 months ago

Thinking a bit more about this, we could add an overload for nbFile that only takes the filename (Instead of filename and content). Then we don't have to create a separate nbReadFile. The one which runs the external code as well would need to be separate, though.

How is this to progress from here? Await feedback from pietroppeter and based on that somebody (me? you? peter?) opens up an issue in nimib?

Apart from the names of the functions, I would that we are ready to start working on this. Feedback can always come later be adapted into the work. Do you feel tempted to implement this in nimib yourself? With guidance of course if you need it :smile:

PhilippMDoerner commented 9 months ago

Do you feel tempted to implement this in nimib yourself? With guidance of course if you need it πŸ˜„

Not really and I don't see myself having any in the near future.

I'm currently like 4 PRs deep in owlkettle, 2 more in the pipeline once a blocking one is merged and 3 more issues I want to tackle. That is before adding more examples for the individual widgets there and making myself a list of widgets that are missing.

The entire thing dawned on me solely because I'm currently rather active throwing stuff at owlkettle and improving the entire doc situation there even further to make it as smooth an entry point as it can be.

HugoGranstrom commented 9 months ago

That's totally understandable 😁 I'll see if I manage to get any work done on this during the weekend πŸ‘

pietroppeter commented 9 months ago

well, I would say thanks for the input @PhilippMDoerner, that's useful and it is perfectly fine to give feedback like this without any need of committing any work. We do ask for politeness though ;)

I agree with @HugoGranstrom that an overload for nbFile that implicitly reads instead of writing a file looks like a good idea.

PhilippMDoerner commented 9 months ago

Errrr was I impolite anywhere? That was not my intention, apologies if I formulated some piece of text badly anywhere. If I did please say so I'll try to keep more of an eye on it.

HugoGranstrom commented 9 months ago

Errrr was I impolite anywhere? That was not my intention, apologies if I formulated some piece of text badly anywhere. > If I did please say so I'll try to keep more of an eye on it.

I didn't perceive you in any bad way at all. I think he just said it as a general remark against the not-always-so-polite tone that some people have in parts of the community :shrug:

pietroppeter commented 9 months ago

No no it was all fine. I just wanted to highlight the fact that feedback through issues is appreciated. Sorry if that sounded defensive in some way

pietroppeter commented 9 months ago

we do ask for politeness

Ah, I understood! It is us that we ask if someone is interested to submit a PR because we try to be polite. I was not asking for politeness. I guess I used a wrong English expression translating directly from Italian. Haha sorry for the misunderstanding. πŸ˜Άβ€πŸŒ«οΈ

PhilippMDoerner commented 8 months ago

I did some staring at the code as I'm waiting for real life to give can.l some spare time to look at PRs :smile: .

Doesn't it basically suffice in nimib to have this?:

template nbFile*(name: string) =
  let fileContent = readFile(name)
  newNbSlimBlock("nbText"):
    nb.blk.output = text

Like I have no clue what any of this does bar the template call, but basically this just replicates the nbText template but fetches the contents of a file from a given string-filepath beforehand.

If I knew how in nimib I'd test it out, so I don't know if that actually works, but my best guess is that it should. It shouldn't clash with the other nbFile templates either because:

1) it doesn't have a second parameter unlike template nbFile*(name: string, content: string) = 2) it doesn't have a body so it doesn't clash with template nbFile*(name: string, body: untyped) = . Or rather, it shouldn't have a body in my mind, since I imagine the syntax calling it would be nbFile("/some/path/blub.nim").

That seems pretty straightforward. So the necessary steps then would be:

pietroppeter commented 8 months ago

yes, it should be something almost as simple as that. to have the same output of the other nbFile blocks it should be:

template nbFile*(name: string) =
  ## Read content of filename
  newNbSlimBlock("nbFile"):
    nb.blk.context["filename"] = name
    nb.blk.context["ext"] = name.getExt
    nb.blk.context["content"] = readFile(name)

I have adapted the code in https://github.com/pietroppeter/nimib/blob/dc890600a2274aaec8da71f5e16be084a7f15614/src/nimib.nim#L129C1-L137C1

The reason why we would use newNbSlimBlock("nbFile") instead of newNbSlimBlock("nbText") is that we want the content rendered differently, as in here: https://pietroppeter.github.io/nimib/allblocks.html#nbfile

The rendering code of nbFile (in the default html backend) for reference is this one: https://github.com/pietroppeter/nimib/blob/dc890600a2274aaec8da71f5e16be084a7f15614/src/nimib/renders.nim#L33

Indeed tests are desirable for nimib, let me quote the steps in nimib's CONTRIBUTING.md for adding a new block:

  • add the logic in nimib.nim
  • add the rendering in nimib\renders.nim
  • add some test in tests (if it make sense)
  • add an example in allblocks.nim

Next steps are as you mention, actually we should just release a new nimib version and I would not bump the version in nimibook (this new block is not strictly required in nimibook). Instead in your own nimibook you will add the requirement with the new nimib version.

The release of the new nimib version would be up to us after your PR and we will follow the process outlined here: https://github.com/pietroppeter/nimib/blob/main/changelog.md (we should actually have a release since I just checked and the last one was in march and there have been a few contributions since then, it has been a messy period in the last few months...).

Thanks for pinging and giving us feedback on this! :)

pietroppeter commented 8 months ago

we do ask for politeness

Ah, I understood! It is us that we ask if someone is interested to submit a PR because we try to be polite. I was not asking for politeness. I guess I used a wrong English expression translating directly from Italian. Haha sorry for the misunderstanding. πŸ˜Άβ€πŸŒ«οΈ

and I hope you saw that the "politeness" remark was not directed at you. It was totally a communication blunder on my side (see the reply here https://github.com/pietroppeter/nimibook/issues/85#issuecomment-1745553750) that I just quoted here above). You have been absolutely very polite and helpful all the way through, I am sorry if my mistake caused you some discomfort! πŸ™‡

PhilippMDoerner commented 8 months ago

No worries regarding the politeness, I was just too lazy to reply after reading and acknowledging it :smile:

Note that it appears impossible to run the docs task. It doesn't run even with happyx installed, neither after running nimble docsdeps.

PhilippMDoerner commented 6 months ago

With the nimib PR merged, it's basically only a matter of first nimib, then nimibook releasing a new version, would that be correct?

HugoGranstrom commented 6 months ago

Only a nimib release is necessary. Nimibook will use the version of nimib that is the most recent (I think).