nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor #18

pietroppeter · 2021-05-31T05:10:53Z

Clonkk · 2021-05-31T07:58:18Z

src/nimibook/defaults.nim

-  var toc = load("../book/toc.json")
+  let tocPath = "../book/toc.json"
+  if not tocPath.fileExists:
+    withDir("..".AbsoluteDir): # todo: add a withDir for RelativeDir in nimib /paths


"..".AbsoluteDir

Isn't ".." a relative dir and not an absolute dir ?

Could that be replaced by getProjectPath.parentDir() (or maybe currentSourcePath.parentDir()) or will that pose problem when distributing the binary ?

yes, it is, see the comment. I like to use withDir which I added in nimib /paths; it came from nimscript and was added in fusion which I copied and pasted into nimib (since I was not sure how to put fusion requirements in nimble file). I want to add also a withDir with relative directory, but until now it is not there, so now it looks bad (at least it is good that you were able to notice it).

Regarding the other proposals, the rationale is that, nimib will currently use as homeDir docs and it would by default change currentDir to homeDir so a ...RelativeDir should always work with thefolder structure we plan nimibook to have. For your other suggestions maybe getProjectPath could work (does it get the dir of nimble file?), currentSourcePath probably not (since source could be in subfolders of book).

Anyway I will need to revise how paths are handled in nimib, since nimibook has challenged the choices I made, so the idea is: 1) for now have workarounds in nimibook; 2) understand a better design and implement in nimib; 3) update nimibook with the improved design from nimib

pietroppeter · 2021-05-31T20:28:39Z

ok PR is ready for review. I will leave it open for a bit if anyone wants to give an additional look, but I plan to merge tomorrow not after the publication of "This Month With Nim".

HugoGranstrom · 2021-05-31T21:39:11Z

I skimmed through the code and didn't find anything obviously wrong but I'm not as familiar as @Clonkk with the codebase. I've checked that it compiles on my machine and produces reasonable outputs. Thought at first that the code formatting was off but I then noticed you inlined some code in the nbText and had noted it in a comment.

Clonkk

I added my review. It's mostly some small interrogation or minor stuff that could be fixed in later PRs.

In a nutshell :

Should book be dependent upon stuff outside of itself ?
I understand the appeal to auto-document, but I think it would be better to do so by using import nimibook as a library instead of relying on relative ../. CI based on relative folder can also be deceiving.

The changes are overall very good otherwise.

Clonkk · 2021-05-31T22:00:05Z

book/configuration.nim

+# the following is very dependent on how source is written
+proc readBookConfigFields: seq[string] =
+  # current folder is docs
+  let src = "../src/nimibook/types.nim"


Unless I misunderstood something, why does the book rely upon parsing a file in src ?

I think we should keep book and src separate as much as possible.

Also relative path based on "../" will break with Nimble; but that may be something to fix later on

I was also confused about the src/book thing until I realized the file wasn't some kind of configuration file butit rather just one of the pages in the book which did some parsing of the doc-comments in src. 🤦‍♂️

This book being the documentation of what happens in src, I think it is fair to make book depend on src to avoid repeating stuff and keep documentation up to date. The moment you add a new field to Book object implementing another feature you will get the corresponding documentation. For sure we should avoid having src depend on book folder. book folder should be forgotten when installed by nimble so the relative paths there should not matter.

Regarding the implementation, it is indeed a bit hacky (main drawback being that how you add and document fields in Book object impacts the documentation), but I think it is also rather straightforward. Other options I had in mind for implementation of this autodocumenting configurations were:

nim jsondoc ... and parse json file (will also depend directly on src)

use macros.getImpl and parse AST (this depends only on import nimibook, but since we will not have nimibook as a dependency here, it will still ultimately depend on src)

The only option to not depend on src folder is to duplicate documentation in code and book and I would prefer to avoid this unless there are specific issues.

Options 2 was what I had in mind.
Use Nimibook as a library and parse the AST to obtain information but I understand your reasoning.

I think in the long run we need a macros so that we can extract information from the AST for using Nimibook as a API documentation generator (replacement of Nim doc).
Macros based will also allow us to get runnableExamples

But it's a rather complex feature so maybe I think we should merge this first and improve upon it later.

Clonkk · 2021-05-31T22:05:18Z

book/index.nim

-"""
-nbCode:
-  echo "hello mdbook!"
+nbText: "../README.md".readFileUntil("<!--SKIP")


Is it a good idea to have book be dependent on content outside its own folder ?

see remarks above, but also reading README I think is rather harmless. I was planning to have distinct contents only for README.md and index.nim but in the end I did not use that feature.

Yeah, alright; for a static Readme file it's fair game.

Clonkk · 2021-05-31T22:15:00Z

src/nimibook/books.nim

+  when nimvm:
+    s.add '"'
+    for c in v:
+      case c:


Can this be simplified with the use of :

addEscaped

addQuoted

escape / unescape

I didn't analyzed it in details, but it seems like we could simplify it ?

this and the following I think it should be better addressed in this PR to jsony. I ran into the issue while dumping Book object, found the fix and directly applied it. When jsony merges the fix we should remove this and update jsony requirement.

Clonkk · 2021-05-31T22:15:33Z

src/nimibook/books.nim

+    ss.add '"'
+    for c in v:
+      case c:
+      of '\\': ss.add '\\', '\\'


This should be a function if it's used more than once. DRY.

pietroppeter · 2021-06-01T06:41:42Z

Thanks all for the reviews.

In a nutshell :

Should book be dependent upon stuff outside of itself ?
I understand the appeal to auto-document, but I think it would be better to do so by using import nimibook as a library instead of relying on relative ../. CI based on relative folder can also be deceiving.

my thoughts on this (as reported below also in a comment):

This book being the documentation of what happens in src, I think it is fair to make book depend on src to avoid repeating stuff and keep documentation up to date. The moment you add a new field to Book object implementing another feature you will get the corresponding documentation. For sure we should avoid having src depend on book folder. book folder should be forgotten when installed by nimble so the relative paths in there should not matter.

I do not see any other option to not depend on src folder other than duplicating documentation in code and book and I would prefer to avoid this unless there are specific issues.

I will go ahead and merge, feel free to open an issue and argue more on this if my arguments were not convincing (I might change my mind in the end...) :)

new content for book + remove include

d38b3a1

pietroppeter mentioned this pull request May 31, 2021

[RFC] How to deploy Nimibook ? #12

Closed

pietroppeter added 2 commits May 31, 2021 09:22

add informal definition of nimib documents

5354f4b

fix draft not indented

f178707

Clonkk reviewed May 31, 2021

View reviewed changes

pietroppeter changed the title ~~new content for book + remove include~~ nimibook docs + 2 fixes + 1 feature (cfg) May 31, 2021

pietroppeter changed the title ~~nimibook docs + 2 fixes + 1 feature (cfg)~~ nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor May 31, 2021

pietroppeter added 8 commits May 31, 2021 20:05

add newlines

5efe688

rename favicon_whale -> favicon_escaped

e3e6bf1

add book object

7cd7af7

refactor using Book instead of Toc

461d9ed

refactor to use book configuration (hard time fighting circular deps)

5a20022

fix in path_to_root (I broke it earlier)

99cdfbb

add example toc structure

254e86a

add configuration docs

663e4a0

Clonkk reviewed May 31, 2021

View reviewed changes

pietroppeter merged commit 4c8b542 into main Jun 1, 2021

pietroppeter deleted the add-docs branch June 25, 2021 15:35

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor #18

nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor #18

pietroppeter commented May 31, 2021 •

edited

Loading

Clonkk May 31, 2021

pietroppeter May 31, 2021

pietroppeter commented May 31, 2021

HugoGranstrom commented May 31, 2021 •

edited

Loading

Clonkk left a comment •

edited

Loading

Clonkk May 31, 2021

Clonkk May 31, 2021

HugoGranstrom May 31, 2021

pietroppeter Jun 1, 2021

Clonkk Jun 1, 2021 •

edited

Loading

Clonkk May 31, 2021

pietroppeter Jun 1, 2021

Clonkk Jun 1, 2021 •

edited

Loading

Clonkk May 31, 2021

pietroppeter Jun 1, 2021

Clonkk May 31, 2021

pietroppeter commented Jun 1, 2021

nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor #18

nimibook docs + 2 fixes + 1 feature (cfg) + 1 refactor #18

Conversation

pietroppeter commented May 31, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroppeter commented May 31, 2021

HugoGranstrom commented May 31, 2021 • edited Loading

Clonkk left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clonkk Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Clonkk Jun 1, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pietroppeter commented Jun 1, 2021

pietroppeter commented May 31, 2021 •

edited

Loading

HugoGranstrom commented May 31, 2021 •

edited

Loading

Clonkk left a comment •

edited

Loading

Clonkk Jun 1, 2021 •

edited

Loading

Clonkk Jun 1, 2021 •

edited

Loading