search

SciNim / getting-started

Getting started with Nim for Scientific Computing
https://scinim.github.io/getting-started/
Creative Commons Zero v1.0 Universal
61 stars 6 forks source link

Update to nimib 0.3 and nimibook 0.3 #47

Closed pietroppeter closed 2 years ago

pietroppeter commented 2 years ago

Nimib 0.3 is yet unreleased but before release I want to make sure scinim/getting-started will continue to work.

HugoGranstrom commented 2 years ago

Something I'm noticing in the preview is that there are square brackets [] surrounding content of each page and code blocks and images are surrounded by commas ,. Feels like some artifacts from a list. Do you know what this might originate from?

pietroppeter commented 2 years ago

Yep, noticed too and found the reason yesterday. In nimib I changed the part of template that renders the block from

{{&blocks}}

to

{{#blocks}}
{{&.}}
{{/blocks}}

(Before blocks was created in context as a string concatenating single blocks, now it is a sequence)

We need to change that in nimibook too, started to work on it but finished my allotted time. Should have more time today.

pietroppeter commented 2 years ago

work in nimibook has started. fix was very small but I am also doing a lot of much needed maintenance there: https://github.com/pietroppeter/nimibook/pull/50

pietroppeter commented 2 years ago

putting also a fix for latex for units but note that the $..$ in line latex is not converted. in fact it is not supported by mathjax, see https://rust-lang.github.io/mdBook/format/mathjax.html (although there it also says that $$ is not supported but it seems to work)

pietroppeter commented 2 years ago

the work on this PR is done, except that it is currently depending on the development versions of nimib and nimibook. Once I have the official releases I will update the dependencies.

HugoGranstrom commented 2 years ago

putting also a fix for latex for units but note that the $..$ in line latex is not converted. in fact it is not supported by mathjax, see https://rust-lang.github.io/mdBook/format/mathjax.html (although there it also says that $$ is not supported but it seems to work)

The way I read it is that it's a mdBook problem, and not a MathJax problem. Also I would prefer if nb.useLatex enables this so that all themes activate latex the same way. (I use nb.context["latex"] to activate revealjs's latex in nimiSlides for example)

pietroppeter commented 2 years ago

The way I read it is that it's a mdBook problem, and not a MathJax problem. Also I would prefer if nb.useLatex enables this so that all themes activate latex the same way.

could be. agree that it would be better to use a unique interface among themes, it should not be hard to support it indeed in the same way we do it in nimib, I will see if I am able to while documenting and releasing

pietroppeter commented 2 years ago

now latex should use latex and activated in the standard way, taking advantage of the recent change in https://github.com/pietroppeter/nimibook/pull/52, see also https://pietroppeter.github.io/nimibook/latex.html

pietroppeter commented 2 years ago

what do you think of also changing all chapters that do not contain nim code to markdown? it should be basically all index.nim. It should make the CI slightly faster and the result would be the same.

HugoGranstrom commented 2 years ago

what do you think of also changing all chapters that do not contain nim code to markdown? it should be basically all index.nim. It should make the CI slightly faster and the result would be the same.

I won't say no to a faster CI 😎 and it feels unlikely that we will put any actual Nim code in them in the foreseeable future.

pietroppeter commented 2 years ago

done.

unlikely that we will put any actual Nim code in them in the foreseeable future if we do it is easy to get back to nim files.

HugoGranstrom commented 2 years ago

Thanks!

if we do it is easy to get back to nim files.

Indeed :)

Clonkk commented 2 years ago

Should we merge this ? @HugoGranstrom approved it and it seems finished.

pietroppeter commented 2 years ago

the thing is that neither nimib nor nimibook have officially released the 0.3, that is why here we install from head. It was something I was supposed to do and then merge this, then I had a long period of not being able to work on nim stuff and I am catching up now. Releasing nimib 0.3 and nimibook 0.3 is now top of my list of priorities so I guess this could probably wait a little bit more time.

HugoGranstrom commented 2 years ago

I think what we are waiting for is for nimib and nimibook 0.3 to be released. But it should be fine depending on #head as we do for now and change it once they are released

pietroppeter commented 2 years ago

and to be honest what is lacking for release of nimib 0.3 and nimibook 0.3 is essentially documentation. I could in principle tag and update documentation later... let's say that if by this weekend I do not complete the documentation part I will just tag what is there. and then I will update this pr with correct versions and we could merge.

the relevant open issues that I need to tackle are:

pietroppeter commented 2 years ago

I think what we are waiting for is for nimib and nimibook 0.3 to be released. But it should be fine depending on #head as we do for now and change it once they are released

or we could do it like that, but we risk of breaking nimib/nimibook head and this would implicitly break this...

Clonkk commented 2 years ago

and to be honest what is lacking for release of nimib 0.3 and nimibook 0.3 is essentially documentation. I could in principle tag and update documentation later... l

No pressure, I just wanted to know the status of the PR before it becomes stale and we forget

pietroppeter commented 2 years ago

yes, sure. indeed it started having conflicts with main (since @Vindaar added the ecosystem overview). I did resolve the conflict and merge the change. incidentally I changed the way that section was added (instead of section(name, filename): discard a normal entry(name, filename)). Now at least it will make another run of CI and preview... although I think merging conflict I added to the TOC the overview line and I did not merge with recent master, so I would expect it to fail... urgh!

pietroppeter commented 2 years ago

ah no it worked fine, it merged the whole master branch.

HugoGranstrom commented 2 years ago

Yeah, no need to rush it just for the sake of it. I'm not very up to date on how nimiBook is working nowadays but I could help writing some documentation for nimib at least when I'm done with my nbCodeToJs PR.

HugoGranstrom commented 2 years ago

nimib 0.3 will use nimibCodeAsInSource by default now (see https://github.com/pietroppeter/nimib/issues/94) and I've refactored it to work for more cases (ideally all). I've looked through getting-started and found a few bugs thanks to it but they should all be fixed now. But still, I would appreciate it if someone could throw a quick eye at the code blocks in the latest preview on this PR to see if you can spot any obvious errors that I've missed.

Vindaar commented 2 years ago

I just had a look at the preview and didn't see anything out of order. :+1:

pietroppeter commented 2 years ago

I also gave a look and did not find anything strange. At some point with the new default we could change the documentation comments in standard comments.

An unrelated issue I noticed is that there is no arrow for next in chapter 6.2.2

pietroppeter commented 2 years ago

fair enough.. I am so late on everything! 🙄

pietroppeter commented 2 years ago

although actually the CI broke on not finding some .log file... :( looking into this

Clonkk commented 2 years ago

Ah sorry I assumed everything was fine after asking in interal coms since it hadn't moved for a while and preview looked fine.

I should have consulted you as well

pietroppeter commented 2 years ago

No, this is fine. The issue in CI the end is not linked to this PR, but likely to the fact that an updated version of a library broke a chapter. We probably could have a periodic CI (eg running every weekend) so that we catch this thing early