triplea-game / triplea-game.github.io

TripleA Website
https://triplea-game.org/
GNU Affero General Public License v3.0
12 stars 16 forks source link

Resolve syntax errors to support older version of Jekyll #313

Closed DanVanAtta closed 5 years ago

DanVanAtta commented 5 years ago

Getting various 'syntax' errors on startup using jekyll serve. Errors include things like the following:

  Liquid Exception: Liquid syntax error (line 2): 
Tag '{%- for map in site.maps -%}' was not properly terminated with regexp: /\%\}/ in maps/skins.html

  Liquid Exception: Liquid syntax error (line 5): 
Tag '{%- if map.mapCategory == "BEST" %}' was not properly terminated with regexp: /\%\}/ in maps/categories.html

  Liquid Exception: Invalid syntax for include tag: - Valid syntax:
 {% include file.ext param='value' param2='value' %} in _layouts/default.html

It seems that jekyll is not happy with the "-%}" notation and removing the dash resolves this and allows for the server to startup.

RoiEXLab commented 5 years ago

Deja vu #283 I'll have a look into this, seems to be a different issue

RoiEXLab commented 5 years ago

Ok, looks like Ubuntu is using a jekyll version from 2016 for Ubuntu 18.10. Installing jekyll manually (alternatively using Ubuntu 19.04) resolves this issue: https://jekyllrb.com/docs/installation/ubuntu/

DanVanAtta commented 5 years ago

@RoiEXLab exactly what's the benefit of using the 'dash' syntaxes, it's just whitespace?

At that point the ROI of using a syntax supported by a latest jekyll is not at all worth it.

Otherwise we need a solution that involves more than hacking around with manual installations. Updates to documentation with exact steps needed is desired, but also this adds to maintenance overhead.

DanVanAtta commented 5 years ago

Said another way, fixing whitespace in rendered HTML IMO is worth extremely little, not having to spend time doing manual installation far surpasses that. Sacrificing labor time and ease of setup for whitespace, IMO a pedantic concern, is an incorrect prioritization.

Are there other features that are only supported in a latest jekyll, is this going to be a problem in other ways? Am i correct this is just an issue with whitespace for this specific PR? Repeating this twice and not having actual steps to get up and running is a problem. Saying "manually install" without documentation is not a real solution and I'm concerned about maintenance overhead in the meantime.

DanVanAtta commented 5 years ago

Sry to pile on, but to put it a third way, not supporting old versions of jekyll, particularly a default version, for only the benefit of whitespace is not a valuable enough feature to limit compatibility.

RoiEXLab commented 5 years ago

Well having less whitespace in html will probably enhance loading time for mobile devices slightly. Also the "new syntax" is really old by now. Regarding "supporting old versions": The version used by Github pages should really be the reference here and installing a new version is really justjust 2 commands when installing as root, and 5 commands (in the link I provided in the last post) when installing as non-root.

I don't really expect the ubuntu repositories to be up-to-date at this point, there are so many packages that are a couple of versions behind (python 3.6 instead of 3.7, nodejs 8 instead of 12, just to name a few) so I almost always end up installing them based on an official guide.

DanVanAtta commented 5 years ago

Respect the point of view, but I disagree. The additional steps need:

Even if repo's are old in ubuntu, not supporting old jekyll servers for negligible benefit is not a feature.

RoiEXLab commented 5 years ago

I mean if you're using Ubuntu 19.04 installing jekyll really is that straightforward. Regardless the truth is I don't know if the current template code makes use of a liquid 4.0 feature (the templating engine behind jekyll), which introduced this new syntax, but my concern is that if we rely on forward compatibility, we'll run into problems in the long run, especially because it looks like jekyll 4.0 reached alpha phase and github will eventually update to a newer version, no matter what version ubuntu serves for "old" ubuntu versions.

DanVanAtta commented 5 years ago

Launching from this branch, the site seems fine, I have not spotted any issues and I think I visited all pages by now.

but my concern is that if we rely on forward compatibility

Reasonable concern, but I think this is more a medium term problem and not a long term problem. Long term there will be a Ubuntu 20+, and being on Ubuntu 19 will not be quite as bleeding edge (though it's probably dated at this point).

I'm suggesting that dropping old compatibility now, for the sake of this one feature, is not a good call, particularly as this has been an actual problem a few times now already.

DanVanAtta commented 5 years ago

@RoiEXLab I'd like to close this down and push forward. If you're willing to volunteer to submit a documentation PR of the steps needed to install latest jekyll and will keep it up to date, I don't think I can object to you taking that ownership. In that case please submit the PR and revert this one.

Otherwise I think this kind of thing will become a moot point when Ubuntu LTS is at 19.x. Without other compelling reason, the simplicity of supporting an old version is far more valuable than using the latest and greatest.

RoiEXLab commented 5 years ago

@DanVanAtta Seems like a fair deal to me 👍