-
Notifications
You must be signed in to change notification settings - Fork 1
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
Support liquid strict_variables #25
Comments
Thanks for pointing out this issue and providing a solution @nathanweeks. Feel free to open a PR, otherwise this issue will be included in the upcoming "bring things back to the theme" triage. |
Just noticed this is in the starter repo. I going to move it to the theme repo since that appears to be the origin of some of the errors. |
@nathanweeks I went ahead and made all the code changes necessary for the theme to pass the strict variables test and, honestly, I don't think it's worth it. The code is no longer idiomatic and in some cases it's outright mangled. I can push the changes if you would like to have a look for yourself but it's definitely going to take some persuasion for me to merge these changes into main. |
@nathanweeks I put the changes in the |
🧇 |
@nathanweeks Apparently I really dropped the ball on this one; I just remembered your draft PR that makes the theme pass the The nice thing about the old syntax is that Anyway, let me know if this is still a workable solution for you. |
Of course I looked to see if GitHub has a dropping-ball icon, but apparently not. Alan has opened a Pandora's Box by making me aware of the extensive GitHub icon library. 😃 🎈 (A balloon is sort of a dropping ball, it just goes in the opposite direction.) |
👐 ⚾ |
Good point! I had missed that.
Testing the jekyll-theme-legumeinfo strict-variables branch with jekyll-starter-legumeinfo &
A one-line update to the theme resolves the error:
Thanks! |
Good catch @nathanweeks. This has revealed a bug that I haven't been able to find a way around: Apparently the |
I wasn't familiar with the concept of a "drop"; thanks for the reference. In this case, it's |
Yes, After poking and prodding the I'll update this PR to use that syntax. |
@nathanweeks I pushed a commit to the |
Thanks, Alan! Using the updated strict-variables branch, I get an error here (looks like this use of
|
Good catch @nathanweeks! Apparently the regular expression I used to identify problematic |
Thanks @alancleary, it works now! FWIW I did have to uninstall the jekyll-sitemap plugin (which doesn't work with |
When developing a Jekyll site, it can be useful to set the Liquid strict_variables option to prevent accidentally using an undefined variable (e.g., typo, or when a property that is expected to exist in a data file doesn't actually exist). However, there are a number of places in this theme where a variable / property can optionally exist---checked for with an
if
tag---and the Liquid implementation used by Jekyll has a bug that causes an exception to be thrown when the existence of an undefined variable is checked for in this manner.Enabling strict_variables in _config.yml:
Results in the following error:
A workaround (in most cases) is to use the contains operator.
The text was updated successfully, but these errors were encountered: