Removes quotes from ContentDispositonHeaderValue Name and FileName #901
Conversation
|
Remvoes :-) |
|
Do a quick sanity test on the setters while you're here. See if it should be adding quotes. |
jkotalik
changed the title
|
@jkotalik, thanks for signing the contribution license agreement. We will now validate the agreement and then the pull request. |
|
|
||
| var result = ContentDispositionHeaderValue.Parse(contentDispositionLine); | ||
| result.Name = "hello"; | ||
| result.FileName = "\"example.png\""; |
There was a problem hiding this comment.
I meant what happens when you set a value that requires quotes, like "my file name with spaces.txt"
| } | ||
|
|
||
| [Fact] | ||
| public void HeaderNamesWithQuotes_CheckSettersToNotHaveQuotes() |
There was a problem hiding this comment.
I found that the test I wanted already exists:
There was a problem hiding this comment.
You can remove this, it's redundant
|
@Tratcher This good to go? |
| public StringSegment Name | ||
| { | ||
| get { return GetName(NameString); } | ||
| get { return HeaderUtilities.RemoveQuotes(GetName(NameString)); } |
There was a problem hiding this comment.
What's the reason we do this every time?
There was a problem hiding this comment.
How often will the getter be called @Tratcher ?We can store a local copy after RemoveQuotes has been called once, but it would cause a bit of clutter because only Name and FileName would have these local copies.
There was a problem hiding this comment.
The actual data is stored in the Parameters collection and is directly modifiable. This is just a convenience accessor. Any attempt at caching would have to check that the underlying version hadn't changed.
There was a problem hiding this comment.
Not often. Don't optimize it prematurely.
There was a problem hiding this comment.
This is also turning the StringSegment into a string and back into a StringSegment negating the benefits. GetName should take a StringSegment.
There was a problem hiding this comment.
It should accept and return a StringSegment. At least there'd be no allocations then.
There was a problem hiding this comment.
GetName takes a constant key. It doesn't manipulate that value, it uses it for a lookup.
There was a problem hiding this comment.
Why aren't they leading with a _?
There was a problem hiding this comment.
They are all private const
|
|
||
| // Gets a parameter of the given name and attempts to decode it if necessary. | ||
| // Returns null if the parameter is not present or the raw value if the encoding is incorrect. | ||
| private string GetName(string parameter) |
There was a problem hiding this comment.
line 363 can remove the ToString() now
5 participants
Fixes #446 using part of #533