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

WIP FOR DISCUSSION: Templates: take PR Club content from YAML header #1756

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

harding
Copy link
Contributor

@harding harding commented Jun 26, 2024

Part of #1551

Upgrading the PR Review Club helper ({% include functions/detail-list.md %}) to Hugo is straightforward except for one point:

  • Jekyll allows quoted strings to extend across line breaks, e.g.:

     {% include functions/detail-list.md
          q0="This is a question.
            This is more of the same question"
      %}
    
  • Hugo requires all parts of a string to be on the same line, e.g.:

    {{< functions/detail-list.md
        q0="This is a question.  It must all be on the same line."
    >}}
    

It's trivial for me to go back and squash all previous review club entries to be on individual lines and to write a linter that enforces that for future review club entries. However, I like wrapping and we try to wrap everything else, so it would be a bit inconsistent. There are many possible alternative approaches (and I'm happy to discuss them), but one approach I think could be useful is for the Q&A to be written in YAML. This PR updates the most recent PR Review Club to use that approach, including providing template code that should exactly match our current layout.

Downsides:

  • Q&A will need to be added to the header ("front matter") at the top of the file rather than inline with the location where it will appear

Upsides:

  • Many text editors / IDEs can perform live syntax checks on YAML, hopefully allowing missing quotes, overquoting, or other problems to be caught earlier
  • Easy conversion to Hugo or to any future site compiler we use

Comments @stickies-v @LarryRuane and others?

Copy link
Collaborator

@stickies-v stickies-v left a comment

Choose a reason for hiding this comment

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

This new approach looks great to me, I think decoupling content from formatting is a nice improvement, so if this works well with Hugo it's absolutely a yes from me.

As commented, I would prefer keeping all the review club content in one place, but can work with either approach!

_posts/en/newsletters/2024-06-14-newsletter.md Outdated Show resolved Hide resolved
_posts/en/newsletters/2024-06-14-newsletter.md Outdated Show resolved Hide resolved
@harding
Copy link
Contributor Author

harding commented Jun 27, 2024

Forced pushed to take all of @stickies-v suggestions. Thanks!

@harding
Copy link
Contributor Author

harding commented Jun 28, 2024

Note, moving all of the review club content into the YAML header broke the recap_references_generator.rb logic, as it expected the name of the PR being reviewed to be the first link in the section starting on a line by itself.

Given the open discussion about how to port recap generator to Hugo, I'm just going to leave this PR open as-is for now and come back to it when we've decided what to do about the podcasts.

@jonatack jonatack force-pushed the master branch 3 times, most recently from 38028a8 to 9db038c Compare August 26, 2024 20:43
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.

2 participants