GeezerVison

Should we open an issue against corefx? If corefx supports/fixes this in the future, will we have double quotes?

At this point it would be a breaking change in CoreFX. This is the same behavior for System.Net.Http.MultipartFormDataContent in .NET Framework 4.5 when it was added. I found a blog article form 2013 that references this issue. If they did add this, they would probably do so through a new property in which case we wouldn't be broken.

I found this https://github.com/dotnet/corefx/issues/26886#issuecomment-381697212 though, this begs the question why every browser and curl wraps the name and filename fields values in quotes and .NET does not?

Seems it is important fix so we need to add tests. Maybe use output with -WhatIf?

Ilya (@iSazonov) if you can, please provide a code example... otherwise I have no clue what you are suggesting :)

Since the request output is generated inside the HttpClient code, we can't really access it. Only way I can think of would be to echo the raw response back from WebListener and regex for the fields and quotes. That or create a special endpoint on WebListener and do the regex there. If you try to process the request in ASP.NET as a form, you lose the content disposition headers which is where this is stored.

I'm not sure this warrants the level of effort and code it would take to make a meaningful test for it.

I believe we should have tests for the uploading.
I assumed that we could do verbose output of the form but seems it is impossible. So we need full feature tests. Perhaps James Truher (@JamesWTruher) and Aditya Patwardhan (@adityapatwardhan) have ideas about this tests.

Mark Kraus (@markekraus) I think it would be ok to add a test to weblistener to specifically return the field names and values as raw text. Since the values are controlled by the Pester test, it should know exactly what should be returned without having to resort to regex

Steve Lee (@SteveL-MSFT) The problem is the ASP.NET does not provide whether or not the feild names are quoted when you consume the response as a form. The only way to test that is to echo back the raw HTTP request as a string.

Mark Kraus (@markekraus) I see, so you either get just the name value pairs stripped of the quotes or the entire raw request. It seems the risk is low and the cost is high. I would be fine creating a new issue to add such a test in the future, particularly if it's useful beyond this specific issue.

Added #6794 to track possible adding an echo controller to WebListener.

the cost is high

Mark Kraus (@markekraus) implemented WebListener so fast that I'm still amazed :-)

Dongbo Wang (@daxian-dbw) Do you agree that we use the $"" string interpolation? Usually we use formatting with explicit culture.

I don't have a strong opinion. I don't want to block people from using C# language features :)
When converting an interpolated string to a string instance, the current culture is used, which seems fine in this change. But be careful if you want a culture-invariant string.

Dongbo Wang (@daxian-dbw) Thanks!

Mark Kraus (@markekraus) ConvertTo<T> uses InvariantCulture. I believe we should keep consistency.

Yea. I'm not married to any particular string formater or interpolation method. So, whatever works best. String.Format(CultureInfo.InvariantCulture, "\"{0}\"", ...)?

We could

 = "\"" + ... + "\"";

Sorry for wrong close.

You can also choose culture with the string interpolation:

FormattableString message = $"The speed of light is {speedOfLight:N3} km/s.";
string messageInInvariantCulture = FormattableString.Invariant(message);

See https://docs.microsoft.com/en-us/dotnet/csharp/language-reference/tokens/interpolated

I think that the double formatting superfluous here.
Concatenating enough.
Or we could use String.Create() from .Net Core 2.1 (later).

Ilya (@iSazonov) Does the culture on these even matter? I'm not doing any format transformations, I'm just shoving one string inside another.

I remember that we have already corrected misuses of culture in formatting. So I'm trying to be careful here.
If culture is not important here, we should not use this pattern at all.
Now we have already merged #6718 and can use .Net Core 2.1 features - it is very optimized for string manipulations.
I believe here we could use String.Create(). Samples:
https://msdn.microsoft.com/en-us/magazine/mt814808.aspx
http://source.dot.net/#System.Private.Uri/System/Uri.cs,1412

Ilya (@iSazonov) I'm saying that I believe the culture isn't important here because they are already strings when being interpolated and I am not performing any transformations on it. What I understand happens here is is myString.ToString(CultureInfo.CurrentCulture) That does nothing on a string because it is already a string. It would only affect it if it was some other kind of object or if we were performing a formatting operation on that string. I'm not aware of any instance where it would actually change anything in the string. it's not going to strip or change foreign culture characters.

So I'm asking.. does it even matter here? as far as I can tell, what ever string interpolation, concatenation, creation, and building method would all produce the same strings in the end because we are just dealing with combining strings and not doing any formatting or transformations (those are already done by ConvertTo<String>() before the interpolation).

https://docs.microsoft.com/en-us/dotnet/api/system.string.tostring?view=netcore-2.0#System_String_ToString_System_IFormatProvider_ :

Returns this instance of String; no actual conversion is performed.

So I'm asking.. does it even matter here?

No. The discussion is about code style. Bad style is a open door for camouflaged bugs. Therefore, we require explicit curly braces in the if and explicit culture in formattings - after fixing and discussing the latter we even added manuals to our documentation #4983
Here I wanted to understand whether we can use $"" and now believe that we should not use this.

... why is this bad style? what is wrong with $""? I really don't understand what your objecttion is. You don't like it?

Ilya (@iSazonov) Also, I cannot figure out how you intend to use String.Create() here. Can you provide me with an example relevant to this PR? It seems like it is overkill for string concatenation.

I am very sorry for noisy. We had culture related bugs in string operations.
You can see that we have special helper methods in StringUtil.cs to follow a good style. I believe we should use the best practise.
Your code works right (I hope :-) ) but somebody can use the pattern latter in another place mistakenly.

Also, I cannot figure out how you intend to use String.Create() here.

You could ignore this today. I hope we'll do code cleanups to reduce allocations and use the pattern too.

ok.. so $"" is banned even when used properly? So what is the best way to concat strings in this project then? using + has a perf hit so that's out. String.Concat() is in the same boat and needs multiple calls. String.Create() is confusing (seriously.. what is a TState? why is that not documented anywhere? how exactly would you even use this to concat string?) In this case, String.Format() and $"" are identical. I could use String.Format() with the explicit formatter, but that seems really verbose for just enclosing a string in quotes.

ok.. so $"" is banned even when used properly?

If other maintainers agree that we should always use explicit culture then yes.

Previously we use + for concatinations for few arguments (~< 5). Othervise StringBuilder is preferable.
Currently we can and should use .Net Core 2.1 and C# 7.2 features to decrease allocations.
Now we can use ValueStringBuilder, String.Create(), Span and many new string method overloads.

In the case I'd use String.Create(). Although you can ignore it now and use +.
Sample:

            var name = LanguagePrimitives.ConvertTo<String>(fieldName);
            contentDisposition.Name = String.Create(2+name.Length, name, (Span<char> destination, string str) =>
            {
                destination[0] = '"';
                str.AsSpan().CopyTo(destination.Slice(1));
                destination[destination.Length-1] = '"';
            });

If other maintainers agree that we should always use explicit culture then yes.

Why? Why always use explicit culture? String concatenation does not require it. It is relvant when doing formatting or transformations, but just combining strings it's senseless.

Previously we use + for concatinations for few arguments (~< 5).

If this will unblock this PR so it will be merged, I will change it to use +. I still challenge the logic behind this debate.

In the case I'd use String.Create().

Why? That is a seriously over-complicated verbose and tough to follow piece code just to put " around a string. I get it if we were working with concatenating some large strings or this code was being called so many times that performance really matters. but this is a fairly esoteric feature that wont be used often or heavily and it's more important the the code be readable here than to squeeze every ounce of performance out of it.

this is a fairly esoteric feature that wont be used often or heavily

We never know exactly what users will do.

We never know exactly what users will do.

We do know that this is a form submission style that is not as widely adopted as standard forms. We do know that the intended purpose of this submission method is GUI browser based communications. We do know that some APIs do use this for file uploads, but not all. We do know that while there have been asks for this feature to be added, not many.

If I didn't have my own personal need for it, it may never have even entered consideration to be adopted in the web cmdlets.

Yes, we are not clairvoyant and we cannot know 100% what users will do, but, let's be realistic here: the string concatenation for quoting field names filename values and of this feature is NEVER going to be a performance concern. Especially when it is already associated with slow, long running, and memory intense tasks (uploading data). The few fractions of a millisecond shaved off and memory usage saved by using String.Create() here is severely outweighed by a multitude of other critical performance losses happening at the same time in the same feature.

What I do know for certain is that I will have to look at this code in the future. And very likely, someone else will too. I do know that String.Create() is complex and not easy to read. I do know that I can skim the $"" method and see very quickly what is being done. I do know that It takes me a few seconds to figure out that the code you provided is just wrapping a string in quotes.

If you want to add a string concatenation utility method that allows us to abstract String.Create() throughout the project, I would consider using that. But I don't see the need to add such a thing directly in the web cmdlets. I also don't see any benefit at all in using direct String.Create() calls here either.

Forget about String.Create() - you could use String.Concat() or +.

I do know that I can skim the $"" method and see very quickly what is being done. I do know that It takes me a few seconds to figure out that the code you provided is just wrapping a string in quotes.

This is not obviously for code reviewer. My first impression was that it evidently. Then there were questions. Why we use formatting if we need quoting? Why we use InvariantCulture in ConvertTo and CurrentCulture in $""? What's the trick? Should we worry about allocations? - we just had a discussion that PowerShell sometimes consumes too much resources and we should be more thrifty. As a result, we received a good exchange of views. It was useful for me to know your point of view.

Summing up this discussion I think String.Concat() will be the best pattern here.

In this case, the compiler will translate the + concatenated strings to a call to String.Concat():

Keith Hill (@rkeithhill) What plugin you use in VS Code to see IL?

I use JetBrains' dotPeek. Since String.Concat() has an overload that takes params string[], I assume it take as many as you supply it. But I haven't tested that. Here it is with 8 concats:

BTW the compiler is smart about concatenating string literals:

It would be much appreciated if this PR makes it in for Preview3. Just sayin'. :-)