Handlebars-Net / Handlebars.Net

A real .NET Handlebars engine
MIT License
1.24k stars 214 forks source link

Last letter cut off when the string starts with the separator #504

Closed periklis92 closed 2 years ago

periklis92 commented 2 years ago

Split Enumerator strings would have the wrong length if the string started with the separator. This refers to issue #500 and this fix seems to have solved it.

rexm commented 2 years ago

Thanks for the contribution! Since this bug slipped in without failing any unit tests, can you please add at least 1 so we can guarantee it never happens again?

periklis92 commented 2 years ago

Sure thing!

periklis92 commented 2 years ago

So, I was writing some tests and I noticed something, and I am not sure if it is intended or a side-effect of my change. When I split a string, with no StringSplitOptions, that starts with the separator like "/a/c/d/e" the first element of the Enumerator is an empty string (""), and the rest are "a", "c", "d", "e" as expected.

Update: I looked it up and it looks like the default String.Split() method has a similar behaviour. https://docs.microsoft.com/en-us/dotnet/api/system.stringsplitoptions?view=net-6.0#examples

rexm commented 2 years ago

If you revert your change does it still happen?

periklis92 commented 2 years ago

You are right. It does.

periklis92 commented 2 years ago

Everything seems to be working on my machine and the new test covers the previous issue with the sub-strings. I'm not sure why the build fails with this error: https://github.com/Handlebars-Net/Handlebars.Net/runs/5322245489?check_suite_focus=true#step:5:109

oformaniuk commented 2 years ago

@periklis92 , the problem looks to be in Github actions. See https://github.com/Handlebars-Net/Handlebars.Net/pull/506 for more details. Once the PR is merged you may rebase your branch on top of the latest.

oformaniuk commented 2 years ago

@periklis92 , can you please add a test that covers scenario from the issue?

periklis92 commented 2 years ago

The build seems to work now. I'll fix the code smell and clean the commits before the final push. @zjklee , when you say add a test, you mean regarding the netfx451 on windows 2022 issue, or something else?

oformaniuk commented 2 years ago

@periklis92 , I mean {{.Name}} scenario from the issue.

sonarcloud[bot] commented 2 years ago

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

100.0% 100.0% Coverage
0.0% 0.0% Duplication