Allow trans / endtrans tags

[freezernick]

Closes #124

This contribution is made on behalf of @Effective-Bytes and is funded by @GS1-Switzerland

[MalteJanz]

Hi @freezernick, thanks for reaching out and doing a contribution 👋 . I'm happy to help and I'm fine with including this change in the parser, as we also have some shopware specific parts in there 😅 .

For specific rules regarding drupal and this twig trans syntax: feel free to also open a PR. I'm fine with including it but likely only in a disabled by default state (so you can enable that rule/s in your ludtwig config, but I'm open for a discussion if they don't hurt enabled by default).

Back to this PR: Great work and that you managed to figure out all the details that are needed to extend the parser. I added some comments, mostly smaller things. If you are having trouble with these or any questions, just ask, so hopefully we can get your contribution merged 🙂

[freezernick]

I've added a test and removed the loop from the method.
The env UPDATE_EXPECT=1 cargo test grammar::twig:tags::parse_twig_trans didn't seem to work for me, maybe I missed something...

Putting this in an optional rule would be fine for us / our customer.
While I don't see any direct disadvantages by having this activated by default, it may happen that someone copies a Drupal twig template in their vanilla twig project, run into an error, check it with ludtwig and don't get an error about unknown syntax / tags...

[MalteJanz]

I've added a test and removed the loop from the method. The env UPDATE_EXPECT=1 cargo test grammar::twig:tags::parse_twig_trans didn't seem to work for me, maybe I missed something...

Mh, you can also try just running UPDATE_EXPECT=1 cargo test but that might touch other tests. Anyways you can of course also copy the expected output manually from the failed test...

Putting this in an optional rule would be fine for us / our customer. While I don't see any direct disadvantages by having this activated by default, it may happen that someone copies a Drupal twig template in their vanilla twig project, run into an error, check it with ludtwig and don't get an error about unknown syntax / tags...

Just to clarify, I distinguished between the parser (this PR) and rules (e. g.
https://github.com/MalteJanz/ludtwig/blob/da850a17d0d3b72f0bca94a94b33f25a052e5a36/crates/ludtwig/src/check/rules/twig_logic_and.rs )

The former is always active and ensures all twig source code gets parsed into a syntax tree (can still contain some invalid syntax which gets wrapped up in error nodes, these would also be reported). The latter is the actual "linting" part that checks this syntax tree and makes suggestions (based on rules).

So this PR allows ludtwig to parse these {% trans %} hello world {% endtrans%} blocks with any children. It's up to the rules to annotate something wrong there (e.g. reporting any trans blocks with empty body).

Means if your project doesn't support trans and you still use it in your template, ludtwig itself will not report any errors after this PR gets merged. But that would be fine to me, we also allow for example shopware's syntax extensions like || instead of or, but for that at least we have a rule to suggest sticking to or as that is native twig

But feel free to add those rules in a later PR if you want 😉

[MalteJanz]

So this is basically your TWIG_TRANS element and if I look at the provided twig code I would expect it to contain:

• TWIG_TRANS_STARTING_BLOCK => {% trans %}
• BODY => hello world
• TWIG_TRANS_ENDING_BLOCK => {% endtrans %}

Right now it contains TWIG_TRANS two times (the inner is the starting block) and is missing the BODY wrapper around all child nodes.

[MalteJanz]

As mentioned above, for the syntax nodes I would suggest something similar to the basic twig block:

    // twig block
    TWIG_BLOCK,
    TWIG_STARTING_BLOCK,
    TWIG_ENDING_BLOCK,
    
     // twig sandbox
    TWIG_SANDBOX,
    TWIG_SANDBOX_STARTING_BLOCK,
    TWIG_SANDBOX_ENDING_BLOCK,

So after seeing how the twig code for this looks like I would propose:

     // Drupal Trans
    TWIG_TRANS,
    TWIG_TRANS_STARTING_BLOCK,
    TWIG_TRANS_ENDING_BLOCK,

[MalteJanz]

One other thing I forgot to mention, you should add AST structs for these. Have a look here

ludtwig/crates/ludtwig-parser/src/syntax/typed.rs

Lines 523 to 531 in da850a1

	ast_node!(TwigSandbox, SyntaxKind::TWIG_SANDBOX);
	ast_node!(
	TwigSandboxStartingBlock,
	SyntaxKind::TWIG_SANDBOX_STARTING_BLOCK
	);
	ast_node!(
	TwigSandboxEndingBlock,
	SyntaxKind::TWIG_SANDBOX_ENDING_BLOCK
	);

You can also add utility methods if you like (can be useful to work with that AST node in rules that check specific things about these trans blocks), see for example the AST implementation for twig blocks:

ludtwig/crates/ludtwig-parser/src/syntax/typed.rs

Lines 82 to 137 in da850a1

	ast_node!(TwigBlock, SyntaxKind::TWIG_BLOCK);
	impl TwigBlock {
	/// Name of the twig block
	#[must_use]
	pub fn name(&self) -> Option<SyntaxToken> {
	match self.starting_block() {
	None => None,
	Some(n) => n.name(),
	}
	}
	#[must_use]
	pub fn starting_block(&self) -> Option<TwigStartingBlock> {
	support::child(&self.syntax)
	}
	#[must_use]
	pub fn body(&self) -> Option<Body> {
	support::child(&self.syntax)
	}
	#[must_use]
	pub fn ending_block(&self) -> Option<TwigEndingBlock> {
	support::child(&self.syntax)
	}
	}
	ast_node!(TwigStartingBlock, SyntaxKind::TWIG_STARTING_BLOCK);
	impl TwigStartingBlock {
	/// Name of the twig block
	#[must_use]
	pub fn name(&self) -> Option<SyntaxToken> {
	support::token(&self.syntax, T![word])
	}
	/// Parent complete twig block
	#[must_use]
	pub fn twig_block(&self) -> Option<TwigBlock> {
	match self.syntax.parent() {
	Some(p) => TwigBlock::cast(p),
	None => None,
	}
	}
	}
	ast_node!(TwigEndingBlock, SyntaxKind::TWIG_ENDING_BLOCK);
	impl TwigEndingBlock {
	/// Parent complete twig block
	#[must_use]
	pub fn twig_block(&self) -> Option<TwigBlock> {
	match self.syntax.parent() {
	Some(p) => TwigBlock::cast(p),
	None => None,
	}
	}
	}

[MalteJanz]

Suggested change

	let wrapper_m = parser.complete(outer, SyntaxKind::TWIG_TRANS);
	let wrapper_m = parser.complete(outer, SyntaxKind::TWIG_TRANS_STARTING_BLOCK);

[MalteJanz]

Suggested change

	parse_many(
	parser,
	|p| {
	p.at_following(&[T!["{%"], T!["endtrans"]])
	},
	|p| {
	child_parser(p);
	},
	);
	// parse all the children except endtrans
	let body_m = parser.start();
	parse_many(
	parser,
	|p| {
	p.at_following(&[T!["{%"], T!["endtrans"]])
	},
	|p| {
	child_parser(p);
	},
	);
	parser.complete(body_m, SyntaxKind::BODY);

[MalteJanz]

Suggested change

	parser.complete(end_block_m, SyntaxKind::TWIG_ENDTRANS_BLOCK);
	parser.complete(end_block_m, SyntaxKind::TWIG_TRANS_ENDING_BLOCK);