Refactor nb blocks (fix #24) #78

pietroppeter · 2022-01-09T20:43:36Z

this PR changes completely the NbBlock type that is at the basis of Nimib and makes it easier to create custom blocks.

A block will now be created using newNbBlock template (in nimib / blocks, exported) where we assign a command to the block (e.g. "nbText", "nCode", "nbSource") and in order for rendering to be correct, partials and a sequence of renderProcs need to be defined associated to the above command. "Native" blocks and custom blocks are defined in the same way, the only difference is that for native blocks the block is defined in src/nimib.nim and the partials and renderProcs are defined in src/nimib/render while in custom blocks they will likely be defined next to one other.

along with the above changes there are a few welcome add-ons:

a better logging of what happens while running blocks
some tests, sorely needed since ptest was (rightfully) removed
a new partial main

some other accidental or not so welcome changes:

sugar is now exported (accidental)
nb: NbDoc is mutated when rendered (unwelcome, will be changed later)
cannot use both Html and Md backend at the same time (unwelcome, will be changed later)

this feature will be the main highlight of next release (0.3) but in order to merge this PR some stuff needed for release is left out. Among stuff left out:

nbFile is likely broken, should be fixed and tested inside Nimib (and examples should be added)
documentation has not improved and it should
I need to check that (some) projects depending on nimib are not broken (e.g. nimibook, scinim, ...)

after that a release of 0.3 should be in order. Release name will be Block Maker.

During work on this release I have also identified next milestone for 0.4: better handling of render backends (release name backendMaker). In particular:

a big next change would be to use a json data in NbBlock instead of context and generate context during save. In this way you could go back to use multiple render backends at once, document could be made not mutable during save, ...

below my dev notes redacted during this PR

minimal stuff needed to merge:

to be done after merging in devel:

milestone for 0.4: better handling of render backends (release name backendMaker). In particular:

a big next change would be to use a json data in NbBlock instead of context and generate context during save. In this way you could go back to use multiple render backends at once, document could be made not mutable during save, ...

other dev notes:

testing will also be improved (did not notice that ptest is turned off)
(I could also almost pass to an improved doc generation workflow... - problem with README though...)
[breaking change] code will be stripped by default (as a sort of normalization)
- with this change I might able to simplify also tests in tsources? should I normalize in the same way?
all templates have been moved outside of nbInit (no more unused warning!)
log will move in newBlock? will it be newBlock(cmd: string, body, blockImpl: untyped) so that I can call a log at the end?
added render backend as customizable in nbInit
added a context field to NbBlock (inherits partials from nb object)
I cannot have the context to inherit also values from nb object (values not exported and there is no derive in nim-mustache for context, I might want to do a PR)
no need to have a partial field in NbBlock. will render using "{{> " & blk.cmd & "}}" (e.g. {{> nbText}})
actually a partial field is needed if I want to customize (it could be optional). same for a renderPlan for the block
but as first iteration (internal to this PR) I could just use the partials and renderPlans in the doc
later I need to decide if I want them to be Option object (they should be but I never used much that API and not sure if it is worth it) or not
refactored newBlock to contain blockImpl and other identifiers (nbBlock, nbDoc - will this names clash with aliases?)
- also renamed to newNbBlock
- also added simple logging (should I have a whale emoji everywhere instead of [nimib]? yeah probably! but maybe I leave it for another PR...)
not sure whether to use renderPlans or renderPlan for NbDoc (and renderProcs or renderProc?)
code is now normalized (including newlines)
minimal testing for nbText and nbCode added (could be improved later)
note that output currently not stripped and by default it has a newLine at end. should I remove the newline at the end? (we will in rendering...)
not having mdCfg as global object in renders (I would like to have it as const but did not work back then, should check if it works now)
can I remove NbDoc.render field? probably yes
there is no way to print a Context -> follow up PR to nim-mustache
using develop version of nim-mustache with public values
these two do not seem very good ideas, but I do want output without newlines at the end:
- output in context has removeSuffix (with dup)
- exporting sugar now
- especially a bad idea to have blk.output and blk.context["output"] different
- I really should think of an alternative...
something I do not like is that rendering changes state of doc and blocks. it should not (it could affect a second pass with another backend)
in fact now write wants a mutable doc
the moment I have the json backend I might be able to have a non mutable doc (but might not be worth it)
far from home has likely being broken for a while (replacing partial document is wrong given current defaults)
blocks in markdown are separated by empty lines

HugoGranstrom · 2022-01-09T21:47:12Z

Great work, I'm seeing a lot of things here that I'm liking :D This will make theming much easier! There's one thing I'm wondering what the rationale for it is, and that's the removal of NbDoc.render. I'm using it myself in nimiSlides (link) because of the way I decided to structure the slides. I don't see any real disadvantages to keeping it and setting it to renders.render by default, or am I missing something? 😄

pietroppeter · 2022-01-09T23:29:54Z

sorry, I did it again: edited your comment instead of adding a reply. should be back to normal now.

Great work, I'm seeing a lot of things here that I'm liking :D This will make theming much easier!

pretty happy so far, it has really been a long time coming and finally all pieces are falling into place!
and yes, some additional good stuff is coming along (tests!). More than theming I think it will help with making cool custom blocks (html stuff, javascript widgets, ...); release name will be "BlockMaker" (previous release was "ThemeMaker"). But for a "theme" like nimislides I guess it will be useful :)

two main things I am not liking so far:

now rendering mutates document (since block context are updated). It is not great but for the moment I do not think I cannot avoid that
there is blk.context["output"] = blk.output.dup(removeSuffix) (or similar) that is really bad: 1) I now have two different output 2) I now export sugar in nimib (not sure it is really a bad thing, but seems a bit unnecessary). This is mostly due to the need for speed in finalizing this first part (and I do need in some ways to avoid having output with newlines at the end. hopefully I will find a better way.

There's one thing I'm wondering what the rationale for it is, and that's the removal of NbDoc.render. I'm using it myself in nimiSlides (link) because of the way I decided to structure the slides. I don't see any real disadvantages to keeping it and setting it to renders.render by default, or am I missing something? 😄

I think I cannot keep it since nb.render(nb) clashes with nb.render (overload in this case does not work, I had to remove the render field IIRC). It will be a breaking change but honestly the fix is really simple (use nb.render where you previously used nb.render(nb)). And it always seemed weird to me nb.render(nb).

HugoGranstrom · 2022-01-10T09:03:07Z

Haha 😄

pretty happy so far, it has really been a long time coming and finally all pieces are falling into place!
and yes, some additional good stuff is coming along (tests!). More than theming I think it will help with making cool custom blocks (html stuff, javascript widgets, ...); release name will be "BlockMaker" (previous release was "ThemeMaker"). But for a "theme" like nimislides I guess it will be useful :)

Right, had already forgotten about my nbHTML/nbRaw needs 🤭

now rendering mutates document (since block context are updated). It is not great but for the moment I do not think I cannot avoid that

It isn't performant, but couldn't you make a copy of nb.context and mutate it instead and pass it to render?

# old
nb.context["blocks"] = blocks
return "{{> document}}".render(nb.context)

# new
var ctx = nb.context
ctx["blocks"] = blocks
return "{{> document}}".render(ctx)

there is blk.context["output"] = blk.output.dup(removeSuffix) (or similar) that is really bad: 1) I now have two different output.

Yeah, that's not optimal, but it feels like blk.context["output"] is the one that should survive, no?

I now export sugar in nimib (not sure it is really a bad thing, but seems a bit unnecessary). This is mostly due to the need for speed in finalizing this first part (and I do need in some ways to avoid having output with newlines at the end. hopefully I will find a better way.

What's wrong with strip? It can strip at either end using leading or trailing parameters: https://nim-lang.org/docs/strutils.html#strip%2Cstring%2Cset%5Bchar%5D

I think I cannot keep it since nb.render(nb) clashes with nb.render (overload in this case does not work, I had to remove the render field IIRC). It will be a breaking change but honestly the fix is really simple (use nb.render where you previously used nb.render(nb)). And it always seemed weird to me nb.render(nb).

I'm not really sure how that fix helps me 😅 As it is now in this PR, when we call nbSave, it will call write which in turn calls render(doc). In other words, the render function is hardcoded. My problem is that I want to use my custom renderReveal proc for rendering instead of the built-in nimib/renders/render proc. Do you understand my problem?

One solution would be to keep the field but rename it if it collides.

pietroppeter · 2022-01-10T09:29:53Z

It isn't performant, but couldn't you make a copy of nb.context and mutate it instead and pass it to render?

I am thinking something a bit radical but along those lines. Replacing nb.context: Context field with a nb.data: Json field. During rendering first step is to load nb.data into a new context. Context is a weird object to expose directly and I do not really need its specificities. Plus it is really opaque (no public field) and it would require PRs to nim-mustache. I think this will be the way to go. Although it will break stuff (api is similar so providing a context alias should smooth some things out).

Yeah, that's not optimal, but it feels like blk.context["output"] is the one that should survive, no?

given the above it is possible that even code and output field get removed and only survive in the json (with default accessors). Maybe the json is a distinct json and some fields are reserved to be always some type... ok, still not sure how it goes.

What's wrong with strip?

yeah, probably I should just use that (to be honest I only need to strip the last newline, if there is more than one maybe they should stay, mmh not sure). As mentioned I was in a hurry to finish :)

Do you understand my problem?

ah yes, now I do (you actually implemented a new backend) you are right the fix is not easy. But this PR changed fundamentally how render takes place so I think it is supposed to break. What you would need is to create a new backend (basically replace the renderPlan and renderProc). I think I will still change the API there (maybe a backend object by itself). I do have to work at Markdown backend so I will see how that will go. In principle I could keep a renamed field (maybe under a legacy compile time switch) but honestly I do not know how much that is worth. As far as I know only nimislides is using that and it should anyway pass to the new system. Do not worry too much I will help and it will likely not be that much of trouble.

pietroppeter · 2022-01-10T09:30:28Z

and that's very useful feedback, thanks!

pietroppeter · 2022-01-10T09:34:00Z

yeah I briefly check nimislides code and I think it will be much improved by stuff in this PR. honestly for me in scope of this PR is also a PR to nimislides that makes it work (you see in the todo there are a few checks of multiple stuff)

HugoGranstrom · 2022-01-10T10:23:30Z

I am thinking something a bit radical but along those lines. Replacing nb.context: Context field with a nb.data: Json field. During rendering first step is to load nb.data into a new context. Context is a weird object to expose directly and I do not really need its specificities. Plus it is really opaque (no public field) and it would require PRs to nim-mustache. I think this will be the way to go. Although it will break stuff (api is similar so providing a context alias should smooth some things out).

That could certainly work, and it is more general as well in what kinds of types it can handle than Value 👍

given the above it is possible that even code and output field get removed and only survive in the json (with default accessors). Maybe the json is a distinct json and some fields are reserved to be always some type... ok, still not sure how it goes.

That sounds sensible. Don't know about the distinct though, sounds quite complex for little benefit. If code or output isn't strings, just error and shout at the user 🤷

yeah, probably I should just use that (to be honest I only need to strip the last newline, if there is more than one maybe they should stay, mmh not sure). As mentioned I was in a hurry to finish :)

The is a stripLineEnd which does exactly that, but it is in-place... If dup is your only use of sugar I would just copy the input to result and do the inplace operation on it. Yeah priority nr 1 is that it works :)

ah yes, now I do (you actually implemented a new backend) you are right the fix is not easy. But this PR changed fundamentally how render takes place so I think it is supposed to break. What you would need is to create a new backend (basically replace the renderPlan and renderProc). I think I will still change the API there (maybe a backend object by itself). I do have to work at Markdown backend so I will see how that will go. In principle I could keep a renamed field (maybe under a legacy compile time switch) but honestly I do not know how much that is worth. As far as I know only nimislides is using that and it should anyway pass to the new system. Do not worry too much I will help and it will likely not be that much of trouble.

Ahh, "backend" is what it's called. It is sensible that a lot of things will break, but functionality shouldn't disappear. As I understand it, renderPlan and renderProc works on a per-block basis? My use-case is to wrap multiple blocks in a common structure (I don't want a linear seq of the blocks, which is what render gives currently), ie I want to wrap the blocks of different slides in <section></section> which isn't possible right now. Have I understood it correctly? This won't be a problem for the Markdown backend though as it works similarly to the HTML backend with a linear seq of blocks.

One solution would be to have some kind of assemble proc the backend could provide which takes the seq of rendered blocks as input and assembles it and modifies the context appropriately before it is being rendered. For the HTML backend it would simply be:

proc htmlAssemble(nbDoc: var NbDoc, renderedBlocks: seq[string]) =
	nb.context["blocks"] = blocks

proc render*(nb: var NbDoc): string =
  var blocks: seq[string]
  for blk in nb.blocks.mitems:
    blocks.add nb.render(blk)
  nb.assembleProc(nb, blocks) # nb.assembleProc is htmlAssemble
  return "{{> document}}".render(nb.context)

And in nimiSlides case it would basically do what renderReveal does currently but without the rendering part. How does this sound if you don't want the render proc to be customizable?

yeah I briefly check nimislides code and I think it will be much improved by stuff in this PR. honestly for me in scope of this PR is also a PR to nimislides that makes it work (you see in the todo there are a few checks of multiple stuff)

That will be very much appreciated, or at least some guidance on how I can do it myself :D

pietroppeter · 2022-01-10T10:34:30Z

My use-case is to wrap multiple blocks in a common structure (I don't want a linear seq of the blocks, which is what render gives currently), ie I want to wrap the blocks of different slides in
which isn't possible right now.

ok starting to understand better. I think it should be doable with current setup, it would be sort of hack but current implementation is likely a worse hack. for example with a simplified api:

nbSlide:
  nbText: "some text"
  nbCode: echo "and some code"

I would implement it as:

template nbSlide(body: untyped) =
  nbSlideStart
  body
  nbSlideEnd

where nbSlideStart and nbSlideEnd create blocks that render to <section> and </section>.

I think this kind of thinking can be adapted to your other apis.

In the future a better solution will probably be to consider container blocks: blocks that can contain multiple blocks and have their separate rendering. It is something I have being thing since I think it would be useful (e.g. a nbFold section that hides stuff under a <details><summary>..., a nbTabs with multiple tabs...). One step at the time though :)

HugoGranstrom · 2022-01-10T11:14:43Z

ok starting to understand better. I think it should be doable with current setup, it would be sort of hack but current implementation is likely a worse hack. for example with a simplified API:

Yeah you are probably right :/ I really liked the idea of not having to wrap the content in a block, as it allows for a more dynamic creation of slides. And it stopped users from making things slides too nested (they aren't visible if nested more than twice). But I guess it will do. :) The fragments (the only other API I have) already uses this kind of logic, it just has to keep track an index which is a bit annoying. As you say, container blocks is really what would be needed in the future.

I'll incorporate these changes in the BlockMaker update of nimiSlides, I don't dare touch it now until FOSDEM 🤣

pietroppeter added 16 commits January 8, 2022 18:32

new newBlock (with test)

d0fa04f

code is stripped as default

7edae61

imporve tests

4943eac

add dev notes (to be removed at the end)

19c0d17

moved templates outside of nbInit

c58f53b

update dev notes

d02c817

preliminaries to change rendering of block

d2c65f3

newNbBlock refactored (and renamed)

7cfac36

nbNormalize

c6f4c4a

add note \c == \r and \l == \n

44bed19

change name to tests

b992098

add new nbText, nbCode with tests

daa4c1f

only log output if there is output

affb2c8

use always check

087c01a

new renderBlock working with (minimal) test

29f127d

nim r hello works! 🍾

a1b54c0

pietroppeter mentioned this pull request Jan 9, 2022

refactor rendering #24

Closed

pietroppeter added 6 commits January 10, 2022 00:38

mostaccio ok

2a13871

numerical ok

de2c1f6

nolan ok

b430c0e

pythno ok

23adfa1

remove ptest

33cfc2b

general cleanup

aad5961

pietroppeter added 9 commits January 16, 2022 15:42

update nbImage to refactored blocks

9cf278f

minimal plan to just merge

f39d032

cheatsheet fixed

5e97a70

update nimble docs

85238b4

markdown backend + index

821b95b

update nimble docs and rerun

4884b3a

more cleanup and remove NbKind

865e617

finalize notes

98d0474

remove refactor notes

60bf9ad

pietroppeter marked this pull request as ready for review March 6, 2022 15:14

pietroppeter merged commit 07442ee into main Mar 6, 2022

pietroppeter mentioned this pull request Mar 6, 2022

Table of contents with linkable headers #58

Open

pietroppeter deleted the refactor-nb-blocks branch June 27, 2022 06:21

pietroppeter mentioned this pull request Jul 9, 2022

improving docs for v0.3 + fix markdown backend #103

Merged

32 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor nb blocks (fix #24) #78

Refactor nb blocks (fix #24) #78

pietroppeter commented Jan 9, 2022 •

edited

Loading

HugoGranstrom commented Jan 9, 2022 •

edited by pietroppeter

Loading

pietroppeter commented Jan 9, 2022

HugoGranstrom commented Jan 10, 2022 •

edited

Loading

pietroppeter commented Jan 10, 2022

pietroppeter commented Jan 10, 2022

pietroppeter commented Jan 10, 2022

HugoGranstrom commented Jan 10, 2022 •

edited

Loading

pietroppeter commented Jan 10, 2022 •

edited

Loading

HugoGranstrom commented Jan 10, 2022

Refactor nb blocks (fix #24) #78

Refactor nb blocks (fix #24) #78

Conversation

pietroppeter commented Jan 9, 2022 • edited Loading

HugoGranstrom commented Jan 9, 2022 • edited by pietroppeter Loading

pietroppeter commented Jan 9, 2022

HugoGranstrom commented Jan 10, 2022 • edited Loading

pietroppeter commented Jan 10, 2022

pietroppeter commented Jan 10, 2022

pietroppeter commented Jan 10, 2022

HugoGranstrom commented Jan 10, 2022 • edited Loading

pietroppeter commented Jan 10, 2022 • edited Loading

HugoGranstrom commented Jan 10, 2022

pietroppeter commented Jan 9, 2022 •

edited

Loading

HugoGranstrom commented Jan 9, 2022 •

edited by pietroppeter

Loading

HugoGranstrom commented Jan 10, 2022 •

edited

Loading

HugoGranstrom commented Jan 10, 2022 •

edited

Loading

pietroppeter commented Jan 10, 2022 •

edited

Loading