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

Sigil L for safe Slime HTML #63

Merged
merged 6 commits into from
Dec 3, 2018
Merged

Sigil L for safe Slime HTML #63

merged 6 commits into from
Dec 3, 2018

Conversation

the-mikedavis
Copy link
Contributor

Closes #61. Sorry about the wait.

This is pretty much the same as in EEx. It just takes the extra step to do the slime precompilation. There is one snag that the precompilation causes. For either ~e or ~E sigils in EEx, interpolation isn't allowed. The precompilation doesn't prevent interpolation, so if we use ~L, we can interpolate. I think it's kind of nice to be able to interpolate, but it might be a little confusing if someone wanted to use the #{@assign} syntax in the template.

Copy link
Member

@doomspork doomspork left a comment

Choose a reason for hiding this comment

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

This is wonderful @the-mikedavis! I want to give @Rakoth a chance to look this over 😁

Copy link
Member

@Rakoth Rakoth left a comment

Choose a reason for hiding this comment

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

Sorry for long wait @the-mikedavis

@doomspork I didn't get any notifications about your review request =(


iex> import PhoenixSlime
iex> ~L"\""
...> p hello #{"world"}
Copy link
Member

Choose a reason for hiding this comment

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

I believe, it should be

Suggested change
...> p hello #{"world"}
...> p hello \#{"world"}

so interpolation is actually present in string at sigil compile time

@doc """
Provides the `~l` sigil with HTML safe Slime syntax inside source files.

Raises on attempts to interpolate with `\#{}`. Use `~L` to interpolate.
Copy link
Member

Choose a reason for hiding this comment

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

I think it is misleading, ~L does not allow to interpolate values, instead interpolation processed by slime parser

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see your point @Rakoth. Do you think we should remove the ~l or just rephrase the documentation?

Copy link
Member

Choose a reason for hiding this comment

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

It is now matches the behavior of the ~E sigil, so lets just rephrase the documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Rakoth I've changed up the documentation and the error message. How does this look?

Copy link
Member

Choose a reason for hiding this comment

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

Great!

@doomspork
Copy link
Member

No worries @Rakoth! I have been unbelievably busy with family and work, I know we all get busy 😁

@Rakoth
Copy link
Member

Rakoth commented Dec 2, 2018

@the-mikedavis do you mind if I squash commits before merge?

@the-mikedavis
Copy link
Contributor Author

@Rakoth 👍

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

Successfully merging this pull request may close these issues.

3 participants