-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
TextLoader method override detection #64192
Conversation
d1379d6
to
55ee1a1
Compare
Done with review pass (commit 1) |
609e19e
to
afe166a
Compare
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.
Done review pass (commit 2)
Done with review pass (commit 1 vs. 2) |
b85b3cf
to
2ee95a9
Compare
2ee95a9
to
ce543d2
Compare
Done with review pass (commit 5) |
b56cf4b
to
50515cd
Compare
bbbc5a0
to
f44d8af
Compare
/azp run |
Azure Pipelines successfully started running 4 pipeline(s). |
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.
LGTM (commit 5)
@@ -88,6 +88,7 @@ private protected virtual SourceText CreateText(Stream stream, LoadTextOptions o | |||
EncodedStringText.Create(stream, DefaultEncoding, checksumAlgorithm: options.ChecksumAlgorithm); | |||
#pragma warning restore | |||
|
|||
[Obsolete("Use/override LoadTextAndVersionAsync(LoadTextOptions, CancellationToken)")] |
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.
Consider explicitly specifying that this is a warning.
/// <summary> | ||
/// Options used to load <see cref="SourceText"/>. | ||
/// </summary> | ||
public readonly struct LoadTextOptions : IEquatable<LoadTextOptions> |
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.
Is there a particular reason not to use a record struct here?
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.
Is there a particular reason not to use a record struct here?
Yes, it generates a lot of things that we don't want
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.
I'm not sure it does here. We seem to be very intentionally using all those things.
97d7a5e
to
d405cd6
Compare
Implements TextLoader API change #63888
Implements pattern used by BCL to deprecate public abstract methods:
abstract
tovirtual
if neededWe can still stack overflow if the old method is overridden but calls the base implementation.