Skip to content

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

Merged
probot-auto-merge[bot] merged 4 commits intoHandlebars-Net:masterfrom
periklis92:issue-#500
Mar 5, 2022
Merged

Last letter cut off when the string starts with the separator#504
probot-auto-merge[bot] merged 4 commits intoHandlebars-Net:masterfrom
periklis92:issue-#500

Conversation

@periklis92
Copy link
Contributor

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.

@periklis92 periklis92 changed the title Issue 500 Last letter cut off when the string starts with the separator Feb 23, 2022
@rexm
Copy link
Member

rexm commented Feb 23, 2022

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
Copy link
Contributor Author

Sure thing!

@periklis92
Copy link
Contributor Author

periklis92 commented Feb 24, 2022

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
Copy link
Member

rexm commented Feb 24, 2022

If you revert your change does it still happen?

@periklis92
Copy link
Contributor Author

You are right. It does.

@periklis92
Copy link
Contributor Author

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
Copy link
Member

@periklis92 , the problem looks to be in Github actions. See #506 for more details. Once the PR is merged you may rebase your branch on top of the latest.

@oformaniuk
Copy link
Member

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

@periklis92
Copy link
Contributor Author

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
Copy link
Member

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

@sonarqubecloud
Copy link

sonarqubecloud bot commented Mar 5, 2022

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

@probot-auto-merge probot-auto-merge bot merged commit 1216aaf into Handlebars-Net:master Mar 5, 2022
Sign up for free to 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.

3 participants