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

Allow to use symbols on the front matter #396

Merged
merged 2 commits into from Oct 18, 2021

Conversation

JuanVqz
Copy link
Contributor

@JuanVqz JuanVqz commented Sep 21, 2021

This is a 🐛 bug fix.

Summary

Add Symbol in the PERMITED_CLASSES array.

Context

Fixes #395

@jaredcwhite
Copy link
Member

jaredcwhite commented Sep 21, 2021

I realize now I badly worded my initial issue. I've added some additional context. This is a great fix @JuanVqz, but we'll need a couple more changes to address the problem I was seeing.

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Sep 21, 2021

I realize now I badly worded my initial issue. I've added some additional context. This is a great fix @JuanVqz, but we'll need a couple more changes to address the problem I was seeing.

I'll add the .to_s statement.

@@ -131,7 +131,7 @@ def convert(content, convertible)
end

def matches(ext, convertible)
if convertible.data[:template_engine] == "erb" ||
if convertible.data[:template_engine].to_s == "erb" ||
(convertible.data[:template_engine].nil? &&
@config[:template_engine] == "erb")
Copy link
Contributor Author

@JuanVqz JuanVqz Sep 22, 2021

Choose a reason for hiding this comment

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

@jaredmoody should I add to_s here too?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you meant @jaredcwhite 😊

Copy link
Member

Choose a reason for hiding this comment

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

should I add to_s here too?

@JuanVqz I'd suggest a broad search for "erb", might need to update a few places.

Probably have the same issue with "liquid" as well.

@jaredcwhite
Copy link
Member

@JuanVqz Do you think this will be ready for me to review soon? I'd be happy to help get it over the finish line. 🙏

@JuanVqz
Copy link
Contributor Author

JuanVqz commented Sep 30, 2021

Yes, please. I think it's ready

@jaredcwhite
Copy link
Member

Hey @JuanVqz, I'll merge in the PR but will also revert the changes to the YAML parsing — I don't want our YAML parser to be able to use symbols. (In the original issue the symbols in question were coming from Ruby front matter, not YAML, so I think keeping YAML string-only is fine.)

@jaredcwhite jaredcwhite merged commit 71a7b23 into bridgetownrb:main Oct 18, 2021
jaredcwhite added a commit that referenced this pull request Oct 18, 2021
@JuanVqz
Copy link
Contributor Author

JuanVqz commented Oct 18, 2021

@jaredcwhite thank you for your feedback

@JuanVqz JuanVqz deleted the fix-symbols-on-front-matter branch October 18, 2021 01:17
@jaredcwhite
Copy link
Member

@JuanVqz no problem — your contribution is much appreciated!

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.

In Ruby front matter, template engine not working as a symbol
3 participants