Abstract out site-wide variables into a single include #178

andrewdavidwong · 2021-06-23T10:49:02Z

Known issues:

variables.liquid is itself still repetitive. I tried a for loop instead of repeating myself but couldn't get it to work.
This increases build time considerably. I'm sure it can be optimized in some way.

tokideveloper · 2021-06-23T11:34:43Z

I tested a bunch of pages and it seems to work. Nice!

The build time is now around three times as long. That's okay for me.

BTW: HTML comments like

<!-- Site-wide variables -->

appear in the resulting HTML file. So, you could use Liquid comments instead:

{% comment %} Site-wide variables {% endcomment %}

_includes/variables.liquid

andrewdavidwong · 2021-06-23T19:56:27Z

BTW: HTML comments like

appear in the resulting HTML file. So, you could use Liquid comments instead:
{% comment %} Site-wide variables {% endcomment %}

IMHO, liquid comments make the source file too cumbersome to read, since there's no syntax highlighting, and it looks exactly like the actual content when visually scanning the file. Since the HTML comments are harmless, I think it's worth just leaving them in, but I'm open to being convinced otherwise.

andrewdavidwong · 2021-06-23T20:31:21Z

After implementing @marmarek's suggested changes regarding the assignment of doc_link and team_link, build time is back to normal (prior to this commit) levels!

andrewdavidwong · 2021-06-23T20:32:28Z

Since the HTML comments are harmless, I think it's worth just leaving them in, but I'm open to being convinced otherwise.

Also, this is yet another reason to minify our output (along with #177).

marmarek · 2021-06-23T21:01:35Z

PipelineRetry

tokideveloper · 2021-06-23T21:20:55Z

After implementing @marmarek's suggested changes regarding the assignment of doc_link and team_link, build time is back to normal (prior to this commit) levels!

I can confirm this. Also, I tested some of the non-translated pages and they seem to be good (I'm also not set up to test the translated website).

andrewdavidwong · 2021-06-23T22:22:38Z

BTW: HTML comments like

appear in the resulting HTML file. So, you could use Liquid comments instead:
{% comment %} Site-wide variables {% endcomment %}
IMHO, liquid comments make the source file too cumbersome to read, since there's no syntax highlighting, and it looks exactly like the actual content when visually scanning the file. Since the HTML comments are harmless, I think it's worth just leaving them in, but I'm open to being convinced otherwise.

Maybe a good compromise would be to rename this file to variables.html, since it contains HTML comments. That might be easier, anyway, since we don't have any other .liquid files. People might forget to use the .liquid extension and use .html out of habit.

andrewdavidwong · 2021-06-23T22:24:16Z

@marmarek, I don't see any actual failures in the CI log, yet it says it failed.

…riables

The file contains HTML comments, and all of our other files, even those that are mostly in liquid with just a bit of HTML, have the .html extension.

andrewdavidwong · 2021-06-23T22:54:21Z

@marmarek, I don't see any actual failures in the CI log, yet it says it failed.

Ok, the new CI log lists a lot of failures. Looks like something went wrong with the doc index...

marmarek · 2021-06-23T23:53:57Z

I have downloaded the built site (you can do that by clicking "browse" on the right). I'm looking at _site/doc/index.html and I see few issues:

 comment appears 6 times - I guess variables.html indeed got included that many times
the whole doc index is gone:

 <article id="doc-content" class="post-content">

<!-- BEGIN CONTENT -->


<h1>Documentation</h1>


<!-- Check whether the current page's permalink ends in a specific word.
    If it does, include content specific to that page. -->





<!-- END CONTENT -->
<!-- Closing HTML for doc content -->
 </article>

andrewdavidwong · 2021-06-23T23:57:54Z

I have no idea why this is happening.

Apparently I handled this merge commit incorrectly, but I have no idea what I should have done differently.

marmarek · 2021-06-24T00:10:41Z

doc_link variable is not set in the English part of variables.liquid

marmarek · 2021-06-24T00:11:56Z

Maybe it should be outside of that conditional part? It uses lang variable already, so should work in both cases the same.

andrewdavidwong · 2021-06-24T00:13:08Z

doc_link variable is not set in the English part of variables.liquid

Ahhh, I see. Thank you.

Maybe it should be outside of that conditional part? It uses lang variable already, so should work in both cases the same.

Ok, sounds good.

@marmarek

Assign doc_link only once. As @marmarek pointed out on #178, it already uses the `lang` variable, so it should work the same way regardless of language.

andrewdavidwong · 2021-06-24T00:42:30Z

Ah, same thing with the team link. Fixing.

@marmarek

Assign team_link only once. As @marmarek pointed out on #178, it already uses the `lang` variable, so it should work the same way regardless of language.

andrewdavidwong · 2021-06-24T02:04:53Z

Looks like we're all good now! Merging.

tokideveloper · 2021-06-24T06:58:06Z

I have downloaded the built site (you can do that by clicking "browse" on the right). I'm looking at _site/doc/index.html and I see few issues:
* `` comment appears 6 times - I guess variables.html indeed got included that many times

I now see this on the official website. Has this been forgotten or is it intentionally?

Shouldn't it be sufficient to include the variables once in _includes/head.html?

I could open a new pull request for that.

tokideveloper · 2021-06-24T07:44:10Z

Being on the very current commit, I get two build warnings by Jekyll:

Build Warning: Layout 'doc-full' requested in _doc/developer/services/admin-api.md does not exist.
Build Warning: Layout 'doc-index' requested in _doc/doc.md does not exist.

So, what is wrong here? Should the mentioned files have other layouts? Or was it wrong to delete the mentioned layouts?

tokideveloper · 2021-06-24T07:48:54Z

Being on the very current commit, I get two build warnings by Jekyll:

Build Warning: Layout 'doc-full' requested in _doc/developer/services/admin-api.md does not exist.
Build Warning: Layout 'doc-index' requested in _doc/doc.md does not exist.

So, what is wrong here? Should the mentioned files have other layouts? Or was it wrong to delete the mentioned layouts?

Oh, wait, I think the mistake is on my side. I didn't check out the correct submodules/commits, I guess.

tokideveloper · 2021-06-24T08:22:25Z

Shouldn't it be sufficient to include the variables once in _includes/head.html?

I tried it out now and it's a bad idea: Almost everything vanishes. However, it "shows" (in a sense) the snippets that are missing in the data files for translations.

andrewdavidwong · 2021-06-24T10:40:43Z

I have downloaded the built site (you can do that by clicking "browse" on the right). I'm looking at _site/doc/index.html and I see few issues:
* `` comment appears 6 times - I guess variables.html indeed got included that many times
I now see this on the official website. Has this been forgotten or is it intentionally?

Not forgotten. Since {% variables.html %} itself is included once at the top of many files in _includes, and since more than one such file can potentially be included in a page, I don't see how to avoid having {% variables.html %} end up included more than once on a page.

As I said above, the comments themselves are harmless (#178 (comment)), and it's better to remove them by minifying our output (#178 (comment), QubesOS/qubes-issues#6731).

Of course, another option is simply to remove the comments entirely, but I don't think we should uncomment our code just to avoid seeing comments in the unminified output. That would seem perverse.

andrewdavidwong · 2021-06-24T10:51:53Z

I have downloaded the built site (you can do that by clicking "browse" on the right). I'm looking at _site/doc/index.html and I see few issues:
* `` comment appears 6 times - I guess variables.html indeed got included that many times
I now see this on the official website. Has this been forgotten or is it intentionally?
Not forgotten. Since {% variables.html %} itself is included once at the top of many files in _includes, and since more than one such file can potentially be included in a page, I don't see how to avoid having {% variables.html %} end up included more than once on a page.

As I said above, the comments themselves are harmless (#178 (comment)), and it's better to remove them by minifying our output (#178 (comment), QubesOS/qubes-issues#6731).

Of course, another option is simply to remove the comments entirely, but I don't think we should uncomment our code just to avoid seeing comments in the unminified output. That would seem perverse.

BTW, it's not just from variables.html. If you look at the source of any page on the live website, there are many comments from other includes, as well. So, unless we rip out all of the comments I just added, which (as I mentioned above) would be perverse, we're going to have these extra HTML comments that apply to the source code but not to the HTML output until we start minifying.

marmarek · 2021-06-24T10:58:06Z

This comment itself is rather harmless. But it shows the variables.html file is included several times, which may have negative side effects. At the very least, it may make the build slower (which is not a big problem).

andrewdavidwong · 2021-06-24T11:07:35Z

This comment itself is rather harmless. But it shows the variables.html file is included several times, which may have negative side effects. At the very least, it may make the build slower (which is not a big problem).

Right, as I said above (#178 (comment)): Since variables.html itself is included once at the top of many files in _includes, and since more than one such file can potentially be included in a page, I don't see how to avoid having variables.html end up included more than once on a page.

andrewdavidwong · 2021-06-24T11:09:06Z

This comment itself is rather harmless. But it shows the variables.html file is included several times, which may have negative side effects. At the very least, it may make the build slower (which is not a big problem).

Right, as I said above (#178 (comment)): Since variables.html itself is included once at the top of many files in _includes, and since more than one such file can potentially be included in a page, I don't see how to avoid having variables.html end up included more than once on a page.

However, as I also mentioned above (#178 (comment)), the actual build time, in practice, has not increased compared to what it was before.

marmarek · 2021-06-24T11:12:48Z

As long as variables.html is only setting new variables, it should be okay-ish. Once it starts modifying existing ones (like, appending something to an existing var), it will be a problem - the operation will be done twice (or thrice etc).
The idea of including variables.html once at some common, early file (or maybe just include it in layouts but not _includes?) should work, but as @tokideveloper tested, it isn't trivial to get it right.

I don't think we need to fix it now. But IMO we will need it one day. Maybe create an issue with low priority?

andrewdavidwong · 2021-06-24T11:20:47Z

As long as variables.html is only setting new variables, it should be okay-ish. Once it starts modifying existing ones (like, appending something to an existing var), it will be a problem - the operation will be done twice (or thrice etc).
The idea of including variables.html once at some common, early file (or maybe just include it in layouts but not _includes?) should work, but as @tokideveloper tested, it isn't trivial to get it right.

I don't think we need to fix it now. But IMO we will need it one day. Maybe create an issue with low priority?

Done: QubesOS/qubes-issues#6734

The layout idea is a good one. I will experiment with that. (Update: After a quick test, no dice. Same result as @tokideveloper.)

tokideveloper · 2021-06-24T13:50:55Z

Maybe we could leave all the includings of variables.html there but surround the whole variables.html with these lines:

{% if architecture == nil or architecture.size == 0 %}
  ...
{% endif %}

It can also be any other variable that is set in variables.html in both cases (for the official site and for the translated site) but I used architecture in this example since it's a variable that saves whole pages and I experienced that sometimes such variables lose their contents (roughly said).

tokideveloper · 2021-06-24T14:13:07Z

Maybe we could leave all the includings of variables.html there but surround the whole variables.html with these lines:
{% if architecture == nil or architecture.size == 0 %}
  ...
{% endif %}
It can also be any other variable that is set in variables.html in both cases (for the official site and for the translated site) but I used architecture in this example since it's a variable that saves whole pages and I experienced that sometimes such variables lose their contents (roughly said).

It works! 🎉 It saves some build time (from 13 sec to 11 sec on my machine) and also the number of actual variable "updates" has been reduced.

However, I wonder if I should insert these checks not for the whole thing but for every single variable separately. What do you think?

marmarek · 2021-06-24T14:15:41Z

However, I wonder if I should insert these checks not for the whole thing but for every single variable separately. What do you think?

I'd go with a single check for all. Less clutter and still should do the job.

tokideveloper · 2021-06-24T14:19:03Z

Okay, stay tuned for my PR.

andrewdavidwong · 2021-06-24T14:27:04Z

Maybe we could leave all the includings of variables.html there but surround the whole variables.html with these lines:
{% if architecture == nil or architecture.size == 0 %}
  ...
{% endif %}
It can also be any other variable that is set in variables.html in both cases (for the official site and for the translated site) but I used architecture in this example since it's a variable that saves whole pages and I experienced that sometimes such variables lose their contents (roughly said).

I don't understand. architecture isn't even assigned until inside of this, so how is it meaningful as a wrapper around the outside?

Why does this work?

andrewdavidwong · 2021-06-24T14:30:23Z

Maybe we could leave all the includings of variables.html there but surround the whole variables.html with these lines:
{% if architecture == nil or architecture.size == 0 %}
  ...
{% endif %}
It can also be any other variable that is set in variables.html in both cases (for the official site and for the translated site) but I used architecture in this example since it's a variable that saves whole pages and I experienced that sometimes such variables lose their contents (roughly said).
I don't understand. architecture isn't even assigned until inside of this, so how is it meaningful as a wrapper around the outside?

Oh, I guess that's precisely the point? architecture == nil since it hasn't been assigned yet, unless all of variables.html has already been invoked, in which case we don't want to invoke it again? But then why the or architecture.size == 0?

tokideveloper · 2021-06-24T14:42:56Z

Maybe we could leave all the includings of variables.html there but surround the whole variables.html with these lines:
{% if architecture == nil or architecture.size == 0 %}
  ...
{% endif %}
It can also be any other variable that is set in variables.html in both cases (for the official site and for the translated site) but I used architecture in this example since it's a variable that saves whole pages and I experienced that sometimes such variables lose their contents (roughly said).
I don't understand. architecture isn't even assigned until inside of this, so how is it meaningful as a wrapper around the outside?
Oh, I guess that's precisely the point? architecture == nil since it hasn't been assigned yet, unless all of variables.html has already been invoked, in which case we don't want to invoke it again?

Exactly. Sometimes, variables which save pages (like architecture) seem to be emptied or nil-led (or not set yet), so we need to re-invoke the code again.

But then why the or architecture.size == 0?

Just to be on the safe side. 🙂

tokideveloper · 2021-06-24T15:17:38Z

Okay, stay tuned for my PR.

Opened: #179

Abstract out site-wide variables into a single include

76082f2

andrewdavidwong mentioned this pull request Jun 23, 2021

Translation - scripts, the final part #154

Open

marmarek reviewed Jun 23, 2021

View reviewed changes

_includes/variables.liquid Outdated Show resolved Hide resolved

_includes/variables.liquid Outdated Show resolved Hide resolved

andrewdavidwong added 2 commits June 23, 2021 13:25

Assign doc_link more efficiently

613c3dd

Assign team_link more efficiently

59a5d15

andrewdavidwong mentioned this pull request Jun 23, 2021

Minify website output QubesOS/qubes-issues#6731

Open

andrewdavidwong added 2 commits June 23, 2021 15:29

Merge branch 'master' of github.com:QubesOS/qubesos.github.io into va…

c7ebd2b

…riables

Rename variables.liquid to variables.html (#178)

ba5dfcb

The file contains HTML comments, and all of our other files, even those that are mostly in liquid with just a bit of HTML, have the .html extension.

Fix doc_link

a82cfb9

Assign doc_link only once. As @marmarek pointed out on #178, it already uses the `lang` variable, so it should work the same way regardless of language.

Fix team_link

6af03d9

Assign team_link only once. As @marmarek pointed out on #178, it already uses the `lang` variable, so it should work the same way regardless of language.

andrewdavidwong merged commit 47b282e into master Jun 24, 2021

andrewdavidwong added a commit that referenced this pull request Jun 24, 2021

Abstract out path segments (#178)

2e4d258

andrewdavidwong mentioned this pull request Jun 24, 2021

Include variables.html only once per page QubesOS/qubes-issues#6734

Closed

Uh oh!

Abstract out site-wide variables into a single include #178

Abstract out site-wide variables into a single include #178

Uh oh!

Conversation

andrewdavidwong commented Jun 23, 2021

Uh oh!

tokideveloper commented Jun 23, 2021

Uh oh!

Uh oh!

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

marmarek commented Jun 23, 2021

Uh oh!

tokideveloper commented Jun 23, 2021

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

marmarek commented Jun 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewdavidwong commented Jun 23, 2021

Uh oh!

marmarek commented Jun 24, 2021

Uh oh!

marmarek commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

tokideveloper commented Jun 24, 2021

Uh oh!

tokideveloper commented Jun 24, 2021

Uh oh!

tokideveloper commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tokideveloper commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

marmarek commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

andrewdavidwong commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

marmarek commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tokideveloper commented Jun 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tokideveloper commented Jun 24, 2021

Uh oh!

marmarek commented Jun 24, 2021

Uh oh!

tokideveloper commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

Uh oh!

andrewdavidwong commented Jun 24, 2021

marmarek commented Jun 23, 2021 •

edited

Loading

tokideveloper commented Jun 24, 2021 •

edited

Loading

andrewdavidwong commented Jun 24, 2021 •

edited

Loading

andrewdavidwong commented Jun 24, 2021 •

edited

Loading

andrewdavidwong commented Jun 24, 2021 •

edited

Loading

tokideveloper commented Jun 24, 2021 •

edited

Loading