Skip to content
This repository has been archived by the owner on Sep 13, 2022. It is now read-only.

Miscellaneous improvements in System.Text.* #816

Merged
merged 1 commit into from
Oct 12, 2018

Conversation

terrajobst
Copy link

@terrajobst terrajobst commented Jul 13, 2018

Fixes #847

@dotnet/nsboard

@terrajobst terrajobst added the netstandard-api This tracks requests for standardizing APIs. label Jul 13, 2018
@terrajobst terrajobst added this to the .NET Standard vNext milestone Jul 13, 2018
@@ -90,9 +147,6 @@ public partial class Regex : System.Runtime.Serialization.ISerializable
public System.TimeSpan MatchTimeout { get { throw null; } }
public System.Text.RegularExpressions.RegexOptions Options { get { throw null; } }
public bool RightToLeft { get { throw null; } }
//REFEMIT public static void CompileToAssembly(System.Text.RegularExpressions.RegexCompilationInfo[] regexinfos, System.Reflection.AssemblyName assemblyname) { }
//REFEMIT public static void CompileToAssembly(System.Text.RegularExpressions.RegexCompilationInfo[] regexinfos, System.Reflection.AssemblyName assemblyname, System.Reflection.Emit.CustomAttributeBuilder[] attributes) { }
//REFEMIT public static void CompileToAssembly(System.Text.RegularExpressions.RegexCompilationInfo[] regexinfos, System.Reflection.AssemblyName assemblyname, System.Reflection.Emit.CustomAttributeBuilder[] attributes, string resourceFile) { }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This removal of the comment looks like a bug. I'll add them back.

@terrajobst terrajobst requested review from a team and removed request for a team July 24, 2018 20:24
Copy link
Member

@ViktorHofer ViktorHofer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As Wes pointed out removing the OptionalFieldAttribute here is a breaking change.

@jkotas
Copy link
Member

jkotas commented Jul 27, 2018

This is a breaking change

This is reference assembly, not implementation assembly. Binary serialization attributes do not matter in reference assemblies. Removing the attribute here is perfectly fine; it is not a breaking change.

@weshaggard
Copy link
Member

weshaggard commented Jul 27, 2018

This is reference assembly, not implementation assembly. Binary serialization attributes do not matter in reference assemblies. Removing the attribute here is perfectly fine; it is not a breaking change.

I agree I was actually point out to @ViktorHofer that the breaking change is in .NET Core as that attribute doesn't exist in the implementation.

@jkotas
Copy link
Member

jkotas commented Jul 27, 2018

I agree I was actually point out to @ViktorHofer that the breaking change is in .NET Core as that attribute doesn't exist in the implementation.

Binary serialization is not supported for RegEx in .NET Core (https://docs.microsoft.com/en-us/dotnet/standard/serialization/binary-serialization#serializable-types). It is by design that this attribute is missing in .NET Core.

@ViktorHofer
Copy link
Member

You are absolutely right...

@terrajobst terrajobst merged commit 73caf89 into dotnet:master Oct 12, 2018
@terrajobst terrajobst deleted the system-text branch October 12, 2018 22:28
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
netstandard-api This tracks requests for standardizing APIs.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants