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

Fail the build on undefined variables #248

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

AndrewKvalheim
Copy link
Member

Currently if you mistype a variable name or delete a variable that’s still in use Jekyll will silently ignore the error. Want to enable its strict mode to fail the build instead?

Due to Shopify/liquid#1034 working with deliberately undefined variables is a pain but I’d prefer to make the tradeoff.

@@ -28,13 +28,13 @@
<link rel="stylesheet" type="text/css" media="screen" href="/css/app/code_of_conduct.css">
<link rel="stylesheet" type="text/css" media="screen" href="/css/app/ribbon.css">

<title>{% if page.title %}{{ page.title }} | {% endif %}Seattle GNU/Linux Conference</title>
<title>{{ page.title }}{% unless page.title contains site.name %} | {{ site.name }}{% endunless %}</title>
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This now requires each page to have title, partly because I didn’t see a way to check for their existence but also because we should be writing titles anyway.

@altsalt
Copy link
Member

altsalt commented Jun 8, 2021

These changes makes sense to me, but I don't feel confident in doing a code-review. Trust your judgement if they are ready to go live.

@AndrewKvalheim
Copy link
Member Author

I think it’s mostly a matter of preference;

  • If there’s a mistake in an edit, would we rather the site be published as-is or held back for correction?
  • Is it okay that I un-parameterized some content in old blog posts? Last time I did that there were objections.

@altsalt
Copy link
Member

altsalt commented Jun 9, 2021

Gotcha, I am pretty fine with things being held back for corrections, as long as notification is raised. In terms of un-parameterized content, can you give an example? Perhaps I didn't read the past objections as closely as I should have...

@AndrewKvalheim
Copy link
Member Author

#247 initially included fa9c023 to avoid rewriting history with mangled links, but Ben suggested omitting this step, you +1’d his position, Nathan defended it, and I yielded to consensus. I still don’t really understand the objection so it’s hard for me to predict whether it also applies to this PR.

@altsalt
Copy link
Member

altsalt commented Jun 14, 2021

Still going in circles trying to figure this out, I blame some of the double negatives in comments related to that issue...

IMHO, we want parameterization in general, we just don't want to have Freenode linked anywhere. Removing parameters from old posts is A-okay with me. There is some world where the word wasn't parameterized, but just the link, and I think that's where some confusion perhaps came from?

@nhandler
Copy link
Contributor

@AndrewKvalheim What's the status of this PR? Should it be closed at this point?

@AndrewKvalheim
Copy link
Member Author

AndrewKvalheim commented Oct 15, 2022

Available, should we want it. Probably nice to have, but potentially a nuisance for casual editing.

Personally I lean toward keeping slow issues open, but I understand that it’s also popular to hide and deliberately forget about them.

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

Successfully merging this pull request may close these issues.

3 participants