Skip to content
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

variable pa_py_version not substituted at two places #1694

Closed
nikhiljohn10 opened this issue Nov 23, 2020 · 7 comments · Fixed by #1695 or #1731
Closed

variable pa_py_version not substituted at two places #1694

nikhiljohn10 opened this issue Nov 23, 2020 · 7 comments · Fixed by #1695 or #1731

Comments

@nikhiljohn10
Copy link
Contributor

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.

<pre><code>$ pip{{ book.pa_py_version }} install --user pythonanywhere

<pre><code>$ pa_autoconfigure_django.py --python={{ book.pa_py_version }} https://github.com/&lt;your-github-username&gt;/my-first-blog.git

@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 das-g changed the title URGENT BUG FIX NEEDED variable pa_py_version not substituted at two places Nov 23, 2020
@das-g das-g self-assigned this Nov 23, 2020
das-g added a commit that referenced this issue Nov 23, 2020
@das-g
Copy link
Member

das-g commented Nov 23, 2020

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

@nikhiljohn10
Copy link
Contributor Author

@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
Copy link
Member

das-g commented Nov 24, 2020

@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.

@das-g das-g reopened this Nov 24, 2020
@nikhiljohn10
Copy link
Contributor Author

nikhiljohn10 commented Jun 28, 2021

@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
Copy link
Member

das-g commented Jun 28, 2021

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
Copy link
Contributor Author

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
Copy link
Member

das-g commented Jun 28, 2021

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants