Keep Inline HTML tags in the translated text group while ignoring block level HTML tags

[michael-kerscher]

Fixes #125 by keeping inline HTML tags in the translation text group but skip block level HTML tags.
This is possible with the upgrade of pulldown-cmark in #183

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 85.56701% with 14 lines in your changes are missing coverage. Please review.

Project coverage is 90.95%. Comparing base (2b14491) to head (bb97df5).
Report is 2 commits behind head on main.

Files	Patch %	Lines
i18n-helpers/src/lib.rs	76.27%	10 Missing and 4 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #195      +/-   ##
==========================================
+ Coverage   90.27%   90.95%   +0.67%     
==========================================
  Files          12       12              
  Lines        3034     3085      +51     
  Branches     3034     3085      +51     
==========================================
+ Hits         2739     2806      +67     
+ Misses        207      186      -21     
- Partials       88       93       +5     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mgeisler]

Could the matches! macro here be moved up into the match statement above?

That is, instead of a single match arm with

Event::Html(s) | Event::InlineHtml(s) => {

above, have separate Event::Html(s) and Event::InlineHtml(s) arms.

In general, the matches! macro is used rather rarely in Rust since the match or if let statements normally allow you to do the same thing.

[michael-kerscher]

I was hesitant to duplicate the directives::find() call and handling of Skip and Comment.
I'll give it another pass

[mgeisler]

Ah, I see! I didn't think of that 😄

I think changing the two matches! calls here to a nested match statement would be a small improvement. The whole logic here is becoming a little complex and it could very well be ripe for some refactoring (in a later PR).

[michael-kerscher]

Another point: I did not use the usual match event {} expression as I need _ => unreachable() for all other event types in that case and was not sure if the other style would be better.

The explicit unreachable() makes it more visible though so I will go for that.

[mgeisler]

Thanks @michael-kerscher for putting this up! Based on the test changes, it looks great.

@dyoo and @kdarkhan, I would love to have one of you look at this since you've both been working with this code recently.

[kdarkhan]

The fuzz step is failing due to some changes in rust nightly. The failure seems to occur from one of the dependencies.

Until the dependency is fixed, you can temporarily pin to specific nightly version by changing this line to rustup default nightly-2024-05-10.

[kdarkhan]

The rules that determine whether the HTML is block or inline surprised me. I naively assumed that if a piece of HTML is on a separate line, then it is a block, and inline HTML otherwise. Turns out I was wrong. The rules are defined in https://spec.commonmark.org/0.31.2/#html-blocks. Some tags like p or div in a single line are considered to be blocks, while span is an inline.

Here is an example which demonstrate how they differ. Switch to AST tab to view inlines vs blocks.

I believe even with above rules, not splitting inline elements into separate groups is an improvement.

[mgeisler]

Thanks @michael-kerscher!

[mgeisler]

I think it would be good to remind ourselves to remove this. Otherwise others will likely think that this is there for a specific reason.

Let's hope it's fixed in a month 😄

Suggested change

	run: rustup default nightly-2024-05-10
	# TODO: Unpin this after 2024-06-10
	run: rustup default nightly-2024-05-10

[kdarkhan]

I just merged an update to time dep in #198 and the workaround is no longer necessary.

[michael-kerscher]

I switched back to "nightly" only