Skip to content

Add checks for loose closing blocks - #485

Merged
probot-auto-merge[bot] merged 3 commits into
Handlebars-Net:masterfrom
abraham-fox:check-loose-closing-blocks
Dec 30, 2021
Merged

Add checks for loose closing blocks#485
probot-auto-merge[bot] merged 3 commits into
Handlebars-Net:masterfrom
abraham-fox:check-loose-closing-blocks

Conversation

@abraham-fox

Copy link
Copy Markdown
Contributor

Check loose closing blocks on compile time - fixes#484.

Check loose closing blocks on compile time
}
else if (IsLooseClosingElement(item, parentItem, out var looseBlockName))
{
throw new HandlebarsCompilerException($"Loose closing block '{looseBlockName}' was found");

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Dropping this in from my mobile to start a conversation; I can follow up with more specific suggestions: I am not sure the exception message or phrase “loose closing block” is clear or actionable enough by itself for many devs. They would need to look that up to understand it. Can we make it more clear what the problem is?

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

I see your point and I had worries about the wording. What do you think about "An open element was missed for the closing element '/unless'"?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that we're adding more specific compile-time messages, I'm really wishing I'd added the ability to keep line number and source context :)

"A closing element {{/unless}} was found without a matching open element"?

@oformaniukoformaniukDec 29, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Now that we're adding more specific compile-time messages, I'm really wishing I'd added the ability to keep line number and source context :)

@rexm , I've added this some time ago but unfortunately it's not propagated to every corner of the compiler and it still needs some improvement 🙁
In this case, HelperExpression has Context property of type IReaderContext and HandlebarsCompilerException has constructor accepting it as a parameter.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh awesome! I’ll need to look at how that works :)

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

The message was updated. Also, I've made renaming for functions and vars to get rid of some ambiguity.

@sonarqubecloud

Copy link
Copy Markdown

@probot-auto-merge
probot-auto-mergeBot merged commit c4f34b0 into Handlebars-Net:masterDec 30, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

IndexOutOfRangeException during execution of compiled templates

3 participants

@abraham-fox@rexm@oformaniuk