Migrate to System.Text.Encoding.Web #449
Conversation
| return _encoder.Encode(value); | ||
| } | ||
|
|
||
| public void HtmlEncode(string value, int startIndex, int charCount, TextWriter output) |
There was a problem hiding this comment.
Should we change these method names and parameter ordering to match the new BCL APIs? It will make it easier for people to switch between our DI wrapper and the underlying BCL types, but it's more of a break for us.
There was a problem hiding this comment.
Having parity between the wrapper and BCL types makes sense to me. How much work would it be to do it?
|
@muratg Updated with the new names and signatures. It will make the down stream breaks much worse, but is probably better in the long run. |
|
+@rynowak who does not like wrappers. |
|
@davidfowl Removing the wrappers mostly works, except that it's much harder to mock and test because the needed methods aren't virtual. |
| "dnxcore50": { | ||
| "dependencies": { | ||
| "System.Collections": "4.0.11-beta-*", | ||
| "System.ComponentModel": "4.0.1-beta-*", |
There was a problem hiding this comment.
Hmm, is this actually used? I'm curious for what...
There was a problem hiding this comment.
System.Text.Encodings.Web seems to require it.
There was a problem hiding this comment.
Won't the transitive dependency solve that?
|
|
| @@ -154,13 +163,23 @@ public static QueryString Create(IEnumerable<KeyValuePair<string, StringValues>> | |||
| bool first = true; | |||
| foreach (var pair in parameters) | |||
| { | |||
| if (pair.Key == null) | |||
| { | |||
| throw new ArgumentNullException(nameof(pair.Key)); | |||
There was a problem hiding this comment.
InvalidOperationException as this is not an arg.
|
Clarification: My |
| "dependencies": { | ||
| "System.Text.Encodings.Web": "4.0.0-beta-*" | ||
| }, | ||
| "frameworks": { |
There was a problem hiding this comment.
These need to be updated to use generations now
Tratcher
force-pushed
the
tratcher/encoding
branch
from
e7e7032 to
fe56f19
Compare
|
Rebased and updated. Now that we have the right virtuals we can remove the interfaces and the entire WebEncoders.Core package. CommonTestEncoder is also replaced by HtmlTestEncoder, UrlTestEncoder, and JavaScriptTestEncoder, provided directly in the WebEncoders package. |
|
I miss the interfaces, but |
Tratcher
force-pushed
the
tratcher/encoding
branch
from
3ea3b60 to
f2a7001
Compare
Tratcher
force-pushed
the
tratcher/encoding
branch
from
f2a7001 to
be4fb46
Compare
#391 @davidfowl @muratg @Eilon
David asked me to keep the interfaces for DI, so I replaced the encoders with wrappers. Components that were not using the interfaces I change to directly use the BCL types.