-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Proposal: Specialize IA.Builder.ToImmutableArray, IHS.Builder.ToImmutableHashSet #22294
Comments
I agree calling To your suggestion though, does defining an instance method and hiding it effectively hide the extension method from completion lists in the editor? |
It seems silly to have to optimize for this when the usage is so easy to fix. And no, it's not equivalent to
True, although impact would be limited to people who use analyzers.
I just tested. Unfortunately, no (maybe because the extension method is generic, while the instance one in itself is not). However, the instance method is still called, so a warning would be raised. |
If you tested it where the collection library was in your solution, you should be aware that the Why emit a warning though? If we're going to define the method, why not just have it call |
It's for the purpose of consistency-- it's confusing when the same thing is done in multiple ways.
I actually found out about that while I was testing it. Yes, I tested it outside the solution. |
Yes. |
But we have instance methods that match extension methods names all over the place for purposes of specialization. |
Obsolete is too big hammer in general - I would suggest to use this https://github.com/dotnet/corefx/wiki/ApiCompat instead. |
@AArnott Although I would prefer being consistent, I don't have a very strong opinion on this so I'm willing to go with your solution here. Plus as @karelz says (in the wiki) this will cause warnings for existing codebases, which are often treated as errors and can induce breaking changes. @karelz Just a question-- do you really think it's a good idea to let anyone edit the wiki? This repo has 12k stars so it's visited by quite a lot of people. It isn't too hard to imagine some troll just stopping by and deleting everything/posting random content, which could go unnoticed for some time. |
@jamesqo yes, I think it is a good idea. Trolls can be kicked out of GitHub / our org. And there is history, so we can review changes :) ... I prefer easier collaboration over fear from not-yet-observed malicious behavior. |
@jamesqo: yes IMO just defining an instance method that has the desired behavior seems like the way to go. |
@AArnott OK. Have updated the proposal |
@ianhays and me are okay with the updated propsal, so marking it as api-ready-for-review. |
@safern That's great. One thought I had since writing this, when you write public static class CExtensions { public static void M<T>(this C<T>c) { } }
public class C<T> { public void M() { } }
new C().// cursor is here Both public static class IA
{
public static IA<T> ToIA<T>(this IA<T>.Builder source);
}
public static class IHS
{
public static IHS<T> ToIHS<T>(this IHS<T>.Builder source);
}
... |
Sounds reasonable to me. Could you please update the original proposal? |
But the existing ToImmutableArray method is an extension method. Will defining one for a more derived type take precedence in c#? In other languages? |
@AArnott Sorry for the late response.
Of course it will. System.Collections.Immutable already does this trick where it defines overloads of LINQ methods specific to |
@safern I updated the original proposal-- this can be re-marked as ready-for-review. |
@jamesqo can you please expend the names to all added methods? It will be easier to understand the proposed API surface (I couldn't make heads and tales of it in 10s). Thanks! |
@karelz I was a bit lazy; the type names are so long, and at the time I was editing on my phone. I've updated the proposal now that I'm on my laptop. |
Thanks @jamesqo just labeled it as api-ready-for-review :) |
API approved as described in top post. |
Should be fairly straightforward (adding new public API is the most complex part). Anyone wants to pick it up? |
FYI: The API review discussion was recorded - see https://youtu.be/W_r6oG7nnK4?t=99 (10 min duration) |
@jamesqo any interest in doing this one? |
@danmosemsft Can't right now, mark it up-for-grabs please. |
@danmosemsft I will take this one. |
@davidkaya you are welcome. Let us know if you have questions. There are plenty of past PRs that add API and help in our Documentation also. |
Adding extension methods to solve this problem will be ineffective for folks have
Is this still a problem in VS2017? If so, where is the bug on the language service to track this?
Either exception should be considered a genuine bug in the program. And it's not a binary breaking change either. It would only hit folks who recompile their programs against the new API. I wouldn't tend to lean toward maintaining source compatibility for folks who are intentionally doing something that might throw ANE and catch it later as part of their execution flow. |
* Extensions for immutable builders (#21055) Added extensions for immutable builders, which should be prefered over the extensions on IEnumerable because of performance benefits. * Extension for immutable sorted dictionary builder (#21055) * Extensions for immutable builders in the reference api (#21055) * Tests for immutable builder extensions (#21055) * Null check in extensions for immutable collection builders (#21055)
A while ago I was working on a project and whenever I wanted an
ImmutableArray
I would just callToImmutableArray()
on the enumerable. I got so used to doing this, in fact, that I did it forImmutableArray.Builder
s without realizing there was aToImmutable()
method for a long time.Update 8/13/17
Instead of making the new facades instance methods, they will be extension methods. Rationale is given in this comment.
Update 6/16/17
We can add
IA<T>.Builder.ToIA()
,IHS<T>.Builder.ToIHS()
, etc. instance methods that are simply wrappers forToImmutable()
, preventing people who make silly mistakes like me from degrading the performance of their app.Original content
It might be a good idea to add
ToImmutableArray()
toImmutableArray.Builder
and mark it[Obsolete]
, for the sake of enforcing consistency. Similar for all of the other builder types. The[Obsolete]
should not haveerror: true
because we don't want to break existing code, but it will advise the user to useToImmutable()
./cc @AArnott, what do you think?
The text was updated successfully, but these errors were encountered: