DjangoGirls / tutorial

This is a tutorial we are using for Django Girls workshops
http://tutorial.djangogirls.org/
Other
1.53k stars 1.86k forks source link

variable pa_py_version not substituted at two places #1694

Closed nikhiljohn10 closed 2 years ago

nikhiljohn10 commented 3 years ago

Issue description

https://tutorial.djangogirls.org/en/deploy/#configuring-our-site-on-pythonanywhere

In this link pythonanywhere commands are broken. Honkit did not replaces the pa_py_version variables.


image

Suspected cause: Build time error. But passed through tests.

Ref: Here are 2 piece of code which is deployed.

https://github.com/DjangoGirls/tutorial/blob/221695ee4003b6b90ce6a2c99798e5cdc6eee576/en/deploy/index.html#L796

https://github.com/DjangoGirls/tutorial/blob/221695ee4003b6b90ce6a2c99798e5cdc6eee576/en/deploy/index.html#L800

@das-g @ekohl The pushed code look fine. I am not able to figure out why honkit did not render that variable. Any clue? As for now, can anyone push a hardcoded string "3.8" inside deploy section to replace those variables until we figure this out?

das-g commented 3 years ago

Fix proposed in #1695. Feel free to leave a review.

nikhiljohn10 commented 3 years ago

@ekohl The problem is solved for now with the hotfix. But the variable pa_py_version still exists in book.json. We should either remove the unnecessary variable or fix the substitution problem at its root. I think you should have kept this issue open until the actual problem is resolved.

das-g commented 3 years ago

@ekohl The problem is solved for now with the hotfix. But the variable pa_py_version still exists in book.json. We should either remove the unnecessary variable or fix the substitution problem at its root. I think you should have kept this issue open until the actual problem is resolved.

Ah, sorry. The issue being closed way done automatically upon merge based on my commit message.

I agree that either the variable should be removed or the root cause fixed so that the variable can be used. Reopening thus.

nikhiljohn10 commented 3 years ago

@das-g I have found the root cause after a deep dive in to honkit. Inside _book, each page start with index.html which is converted from markdown file. This file is compiled with all the included files. In the case of included files, any book variables inside the code area is replaced with the varialbe value defiend inside book.json. But in the case of README.md(a.k.a. root for index.html), the variables inside code area is not replace and encapsulated inside a pre-code html tags.

So my recommendation is to not use variable inside code area if it is a README.md file. Instead, using include directive to include that code area seperatly inorder to fix this problem. This must be a issue that should be fixed by honkit from it's repository.

I think this issue is resolved without an actual fix since variables in the code areas inside README.md are currently hard coded. But I'll keeping this issue open until the files are restructured or honkit fix it from root.

das-g commented 3 years ago

Thank you for that research, @nikhiljohn10!

But I'll keeping this issue open until the files are restructured or honkit fix it from root.

Good idea. I'd say we restructure as a workaround (as it'd really be good to centralize those versions across translations), unless an upstream fix can be expected soon enough (within some few months.) Has the problem already been reported to Honkit?

nikhiljohn10 commented 3 years ago

Good idea. I'd say we restructure as a workaround (as it'd really be good to centralize those versions across translations), unless an upstream fix can be expected soon enough (within some few months.) Has the problem already been reported to Honkit?

Yes. We can restructure it. I'll analyse the current structure and I'll update here with a proposal. I hope we don't need a new issue to discuss that. Btw, The honkit is reported here -> honkit/honkit#211

das-g commented 3 years ago

We can restructure it. I'll analyse the current structure and I'll update here with a proposal.

Thank you!

I hope we don't need a new issue to discuss that.

A pull request referring to this issue here will suffice.