-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
move css, fonts, js to assets folder; auto install assets folder #19
Conversation
@pietroppeter When having two books EDIT: Okay, this was due to the option IMO, when caching the compiler should be keep information of the path of the file and not just the name... but that's not another issue entirely |
Fair enough |
It would make sense though to have an explicit |
I kept the assets folder I just didn't delete docs but docs/assets instead |
Okay, that should work for a while 😄 |
Other solution would be to have custom static file in a dedicated folder like On another topic, when testing with a dummy book outside of the repo, all the html file are created in the current folder or in book and not in What am I missing for the html to be generated inside |
a few quick notes
|
For automated setup from scratch, you will need the explicit init therefore the init will have to be comitted so automated deployment from What we can do instead is populate
So either
I can regroup them in a
I don't like the dependence on Nimble. Not all books hould be nimble package IMO. How about using : template newBookFromToc*(booklabel: string, rootfolder: string, body: untyped): Book =
initBook(rootfolder)
static:
let path_to_rootfolder = getProjectPath() / rootfolder
putEnv("nimibook_path_to_rootfolder", path_to_rootfolder) and then : proc useNimibook*(nbDoc: var NbDoc) =
# path handling (fix upstream in nimib)
let
nbThisFile = changeFileExt(nbDoc.filename.AbsoluteFile, ".nim")
thisTuple = nbThisFile.splitFile
pathToRootFolder = getEnv("nimibook_path_to_rootfolder")
nbThisDir: AbsoluteDir = pathToRootFolder.toAbsoluteDir
nbHomeDir: AbsoluteDir = nbThisDir / RelativeDir("..") / "docs".RelativeDir
nbSrcDir = nbThisDir
Also, since we now have explicit path (obtained during the compilation of the user book), we can probably simplify some stuff (because we won't need relativeTo anymore) |
Unless I misunderstood somethingI think there's an issue with the init specialization. I assume it's because there is still one in |
Automated setup from scratch you mean like in our CI? I might be understanding wrong our CI but I think what is doing is rebasing on main before generating the documentation (the build step). If we commited in main the result of an "update" step (the init but just to update to more recent assets/templates...), I think the CI for gh-pages should be able to incorporate that and do not need an init step. What I am thinking of is the possible user workflows and their relative frequencies:
this would be needed if we decide to go with init every time.
I did know
yes, that would be ok. I would use
yeah, using the nimble file mechanism is also one of those things I want to rediscuss in nimib (possibly a wrong design decision that we can still reverse). Something like you have there it is a fine workaround for now (if we could avoid an env variable it would be better but I am not sure how). Note though that In summary, agree on most points, only pushing back a little on the |
I'm not a fan of using ENV variables either, feels like some obscure bugs could creep up in the future. Couldn't we just add a field in |
No, I didn't meant like our CI :). Our CI is based on on github actions and can commit to a branch. If I want to use Nimibook on a self-hosted server and automate deployment, I need to be able to generate everything from a zip/tarball/clean git clone; using only Bash (whatever scripting language your OS uses). Conditionnal init that is required but only once make this harder - the workaround would be to manually implement it in each genbook file with
If So it shouldn't really matter if we re-populate assets (beside maybe the 0.2 milliseconds it takes to copy the folders) in the init everytime - with the condition that the init gets skipped for files that already exists (because customization (or we can put customized stuff into another folder)). Realistically, I think what will end up happening IMHO is that we will write this in if initCond: # or when defined(initCond)
initBook To avoid having to modify source code depending on where we are in the deployment process. So why not incorporate it already in the library ? I really don't see the downsides of it.
Yeah, the re-working of
What's the issue with env variables ? It's not persistent after the program ends and multiple program running in multiple shell concurrently will have their own environment (you can check it with multiple |
My bad then, didn't do my research on what it meant in this context. I thought it was environment variables as in I'm ready to take a look at the nimble bug again now |
ok, then I did not understand. I created a new RFC #20 for the design of a nimibook cli. With that design, to generate the book starting from scratch from a git clone of an existing repository would require the following commands:
You are right, env variables like that are good. As @HugoGranstrom was mentioning I think putting a path in Note also that in #20 I propose to drop current mechanism in |
thanks for the honest and thorough feedback. I will take this into account also when reviewing path handling in nimib. For nimibook, it is ok to proceed with current PR as it is. I think the missing part was the override mechanism for nbHomeDir (pietroppeter/nimib#53) which has been merged and released in nimib v0.1.3. |
In the README I think it's worth noting that if you update the TOC you will have to recompile the CLI. It's obvious if you think about how it works but people don't always think 😛 |
Good point ! I'll add it. |
CI is green, the github page deployed on my fork looks good and everything works locally in my machine ! I think we're good to go :). EDIT: A second pair on eye to check document.mustache to check that I didn't forget to add |
@pietroppeter I just realized (CI failures related) nimibookCli uses Basically, this # var book = ... # Use another variable name instead
var my_super_book = newBookFromToc(...):
nimibookCli will break the CLI. So how would you feel aboud Alternatively, we can pass book as a paramters to |
IMO |
Either solution is fine IMO. newBookFromToc(...):
...
book.git_repository_url = "https://github.com/pietroppeter/nimibook"
nimibookCli The other will be : var mybook = newBookFromToc(...):
...
mybook.git_repository_url = "https://github.com/pietroppeter/nimibook"
nimibookCli(mybook) I committed solution n°2 for now (to check CI only fails because of that). |
agree both solution are fine. 2nd is more explicit and ok to proceed with that. |
I think we are good to go, right? should I merge? 🥁 |
thanks @Clonkk for the work on this and the patience. 🚀 🥳 |
PR for #12 with auto deploy of assets folder