Conversation
|
That would not work because the string would still be there in Orchard Core? It would try to still find a string that has that \r\n
If you ignore it then that string will never match any translated PO string. |
Could you please elaborate?
There's no string in OC has |
|
OC or not. When you use the POExtractor it will extract strings in code. @t["Something \r\n"] --> in your C# code will become msgid "Something \n" Then when you try to load translations it will try to find a matching string for "Something \r\n" still which will result in that string to never be translated unless you remove the "\r" from your code. Here, you need to warn the user that you found a "\r" in the code and that you escaped it else the user will never know about it. |
|
We might log a warning when the user is using |
# Conflicts: # src/OrchardCoreContrib.PoExtractor/PoWriter.cs # test/OrchardCoreContrib.PoExtractor.Tests/PoWriterTests.cs
|
@Skrypt, anything to add here, or shall I merge coz I need to bump the package |
|
I asked Copilot for the last review before merge coz I need to bump the version tonight or tomorrow |
There was a problem hiding this comment.
Pull request overview
Updates PO/POT string escaping to address issue #76 where CRLF (\r\n) sequences in source strings can end up as real newlines in generated msgid values.
Changes:
- Adjusted
PoWriter.Escapeto escape CRLF as\\r\\n(instead of escaping\rindependently). - Added a new unit test asserting carriage return handling in
PoWriter.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/OrchardCoreContrib.PoExtractor/PoWriter.cs |
Changes escaping behavior for CRLF/newlines in generated PO output. |
test/OrchardCoreContrib.PoExtractor.Tests/PoWriterTests.cs |
Adds coverage for carriage return behavior in generated msgid. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Text = "Orchard\rCore" | ||
| }; | ||
|
|
||
| // Act | ||
| using (var writer = new PoWriter(_stream)) | ||
| { | ||
| writer.WriteRecord(localizableString); | ||
| } | ||
|
|
||
| // Assert | ||
| var result = ReadPoStream(); | ||
| Assert.Equal("msgid \"Orchard\rCore\"", result[0]); | ||
| Assert.Equal("msgstr \"\"", result[1]); |
There was a problem hiding this comment.
This test asserts that a standalone carriage return is preserved in the emitted msgid (it expects an actual \r control character in the output). A raw \r inside a PO line is typically undesirable and can lead to invalid or hard-to-read .pot files. If the intent is to fix \r\n handling (issue #76), consider adjusting this test to cover CRLF specifically, or to verify that standalone \r is normalized (removed) or escaped instead of being written verbatim.
There was a problem hiding this comment.
@copilot apply changes based on this feedback
There was a problem hiding this comment.
Updated the test in commit 9c494b0: renamed it to WriteRecord_ShouldEscape_CarriageReturn and changed the assertion to verify that a standalone \r is escaped to \\r in the PO output, rather than being written verbatim.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Agent-Logs-Url: https://github.com/OrchardCoreContrib/OrchardCoreContrib.PoExtractor/sessions/2dd9ced8-94a7-48d5-9acc-840734cc3e30 Co-authored-by: hishamco <3237266+hishamco@users.noreply.github.com>
Related to #76
@Skrypt we could ignore
\rinstead of throwing an exception