-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Expose Comment in ZipArchive and ZipArchiveEntry #59442
Changes from all commits
08f2a8b
973fc8c
be54e4a
28467ae
297b9e5
338b184
31a2227
78d7e37
0a29149
b93305b
7496628
a949883
9738c30
5179ea2
0e47de4
18bbcd4
31602b3
0ae8a64
e9d5b67
77b6ee5
8daedcf
1596bb3
3444631
0749c84
b08f0ac
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -383,5 +383,69 @@ internal static void AddEntry(ZipArchive archive, string name, string contents, | |
w.WriteLine(contents); | ||
} | ||
} | ||
|
||
protected const string Utf8SmileyEmoji = "\ud83d\ude04"; | ||
protected const string Utf8LowerCaseOUmlautChar = "\u00F6"; | ||
protected const string Utf8CopyrightChar = "\u00A9"; | ||
protected const string AsciiFileName = "file.txt"; | ||
// The o with umlaut is a character that exists in both latin1 and utf8 | ||
protected const string Utf8AndLatin1FileName = $"{Utf8LowerCaseOUmlautChar}.txt"; | ||
// emojis only make sense in utf8 | ||
protected const string Utf8FileName = $"{Utf8SmileyEmoji}.txt"; | ||
protected static readonly string ALettersUShortMaxValueMinusOne = new string('a', ushort.MaxValue - 1); | ||
protected static readonly string ALettersUShortMaxValue = ALettersUShortMaxValueMinusOne + 'a'; | ||
protected static readonly string ALettersUShortMaxValueMinusOneAndCopyRightChar = ALettersUShortMaxValueMinusOne + Utf8CopyrightChar; | ||
protected static readonly string ALettersUShortMaxValueMinusOneAndTwoCopyRightChars = ALettersUShortMaxValueMinusOneAndCopyRightChar + Utf8CopyrightChar; | ||
|
||
// Returns pairs that are returned the same way by Utf8 and Latin1 | ||
// Returns: originalComment, expectedComment | ||
private static IEnumerable<object[]> SharedComment_Data() | ||
{ | ||
yield return new object[] { null, string.Empty }; | ||
yield return new object[] { string.Empty, string.Empty }; | ||
yield return new object[] { "a", "a" }; | ||
yield return new object[] { Utf8LowerCaseOUmlautChar, Utf8LowerCaseOUmlautChar }; | ||
} | ||
|
||
// Returns pairs as expected by Utf8 | ||
// Returns: originalComment, expectedComment | ||
public static IEnumerable<object[]> Utf8Comment_Data() | ||
{ | ||
string asciiOriginalOverMaxLength = ALettersUShortMaxValue + "aaa"; | ||
|
||
// A smiley emoji code point consists of two characters, | ||
// meaning the whole emoji should be fully truncated | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should add a case where the emoji does not get truncated. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The only case where the string is truncated is when it's length is greater than expected. The contents don't matter. So if a string is smaller than the max length, it should remain untouched, regardless of its contents. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Added. |
||
string utf8OriginalALettersAndOneEmojiDoesNotFit = ALettersUShortMaxValueMinusOne + Utf8SmileyEmoji; | ||
|
||
// A smiley emoji code point consists of two characters, | ||
// so it should not be truncated if it's the last character and the total length is not over the limit. | ||
string utf8OriginalALettersAndOneEmojiFits = "aaaaa" + Utf8SmileyEmoji; | ||
|
||
yield return new object[] { asciiOriginalOverMaxLength, ALettersUShortMaxValue }; | ||
yield return new object[] { utf8OriginalALettersAndOneEmojiDoesNotFit, ALettersUShortMaxValueMinusOne }; | ||
yield return new object[] { utf8OriginalALettersAndOneEmojiFits, utf8OriginalALettersAndOneEmojiFits }; | ||
|
||
foreach (object[] e in SharedComment_Data()) | ||
{ | ||
yield return e; | ||
} | ||
Comment on lines
+428
to
+431
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Move this to the end of the method to avoid mixing what is being reused from what is unique to this method. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I prefer to yield the shared cases first. |
||
} | ||
|
||
// Returns pairs as expected by Latin1 | ||
// Returns: originalComment, expectedComment | ||
public static IEnumerable<object[]> Latin1Comment_Data() | ||
{ | ||
// In Latin1, all characters are exactly 1 byte | ||
|
||
string latin1ExpectedALettersAndOneOUmlaut = ALettersUShortMaxValueMinusOne + Utf8LowerCaseOUmlautChar; | ||
string latin1OriginalALettersAndTwoOUmlauts = latin1ExpectedALettersAndOneOUmlaut + Utf8LowerCaseOUmlautChar; | ||
|
||
yield return new object[] { latin1OriginalALettersAndTwoOUmlauts, latin1ExpectedALettersAndOneOUmlaut }; | ||
|
||
foreach (object[] e in SharedComment_Data()) | ||
{ | ||
yield return e; | ||
} | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should be in
tests
folder.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I don't know why it was created there. Maybe we used to share something with another assembly, but it got removed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah search didn't return the other file that consumes ZipTestHelper. It's the ZipFile.csproj. I'll revert the move of this file.