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

omit tag escape pattern from resulting RawString element output #170

Merged
merged 3 commits into from
Aug 1, 2017

Conversation

softprops
Copy link
Contributor

I think I covered all the cases here. It did seem a bit repetitive but no more repetitive then the existing code for emitting RawStrings.

Ideally I would have liked to find a way to do this in the grammar itself but I couldn't find a clear path for that. I'm new to the pest syntax but familiar with scala's parser combinator library and couldn't find the equivalent of scala's ^^ operator to transform the grammar

src/template.rs Outdated
@@ -377,12 +377,18 @@ impl Template {
{
let (line_no, col_no) = parser.input().line_col(prev_end);
if token.rule == Rule::raw_block_end {
let text = &source[prev_end..token.start];
let mut text = &source[prev_end..token.start];
if text.starts_with("\\{{") {
Copy link
Owner

Choose a reason for hiding this comment

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

hi @softprops , what if the \\{{ is not at beginning of a raw_text, like hello \\{{world}}

Copy link
Owner

Choose a reason for hiding this comment

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

And we can make this a function to avoid duplicated code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I wasn't sure of the grammar only parsed raw text with these at the beginning. I updated the tests, impl, and refactored to a method

src/template.rs Outdated
@@ -135,6 +135,13 @@ impl Template {
}
}

fn unescape_tags<T>(txt: T) -> String
where
T: Into<String>,
Copy link
Owner

Choose a reason for hiding this comment

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

Can we just use &str here to avoid creating a String just for replace?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call. I didn't see str had that impl. I'll update and push that change tonight

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

@sunng87
Copy link
Owner

sunng87 commented Aug 1, 2017

Thank you @softprops for your patient! I will create a release later today.

@sunng87 sunng87 merged commit bdb0be0 into sunng87:master Aug 1, 2017
@softprops
Copy link
Contributor Author

bawler. Ill grab that and pull it into porteurbars so I think I should be set for our company hackathon. Thanks so much for the fast feedback!

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.

None yet

2 participants