-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Inconsistent naming for singleton properties (Shared|Default|Instance) #3493
Comments
Hello Sergio0694, thank you for opening an issue with us! I have automatically added a "needs triage" label to help get things started. Our team will analyze and investigate the issue, and escalate it to the relevant team if possible. Other community members may also look into the issue and provide feedback 🙌 |
I see the argument for all the current names, they're all slightly different cases. I think I mean I see developers grabbing the instance of So, I'm not sure if I see this as a big problem if we want to keep them separate. It also helps for the messengers to not lead developers to think they should be going to go grab the messenger instance and start using it, eh? @azchohfi thoughts? |
Yeah that's a fair point about I was mostly thinking about the |
If we change anything, we should deprecate, instruct with a clear warning message, and then delete in a future version. Lets not directly remove any property which would cause a big breaking change and frustration to devs. |
Ah, naming being the most difficult problem in software engineering strikes back 🤣 If we don't want to deprecate/break things here (which is a good point I can agree with!), I guess then the remaining question for now would mostly just be about what to use for |
It seems to me that Instance, Shared, and Default are all different (but related) things. Instance - A singleton "instance" of a type. Having done a brief check of the code (master & 7.0preview4 branches) it looks like these are all being used as above. |
I'm a bit confused by two of the definitions you used there, could you clarify a bit? In particular:
What do you mean by "collection of multiple sources"? It seems to me that eg.
By this definition, shouldn't |
Yes "Shared" is a special version of 'Instance' that reflects naming used within the CLR.
Oops, yes. I was still under the old assumption that |
Ooh ok, I get what you mean, thanks!
Had to re-read this like 3 times to be sure I was following, but yeah 😄 cc. @michael-hawker if we all agree I can go ahead and rename to |
Describe the issue
We have a bunch of singleton properties in the toolkit, but with inconsistent naming. I'm wondering whether it wouldn't make sense to have them all converge on a single term, to make them more intuitive for developers. Changing the name would obviously be a breaking change, so this seems like a good moment to consider this with 7.0 around the corner.
A few examples:
https:/windows-toolkit/WindowsCommunityToolkit/blob/a7f897616ba9b7d4ac26934f3439335be7830eb4/Microsoft.Toolkit.Uwp/Helpers/SystemInformation.cs#L40
https:/windows-toolkit/WindowsCommunityToolkit/blob/32656db1cdd4ddc25e3a88c297ce4062fe64d2ad/Microsoft.Toolkit.Mvvm/Messaging/StrongReferenceMessenger.cs#L89
https:/windows-toolkit/WindowsCommunityToolkit/blob/1739fb8195f2479772747328c2a755df6b987761/Microsoft.Toolkit.HighPerformance/Buffers/StringPool.cs#L166
For reference, here's the singleton instance
ArrayPool<T>.Shared
from CoreCLR: link.Expected behavior
Given that in all these cases we're dealing with singleton instances that are also thread-safe, hence having a similar role (with the given differences due to them being in different types), I figure maybe a single name should be used in all cases. To be consistent with CoreCLR too and since all instances do share the thread-safety feature too, I'm thinking probably
Shared
?StringPool
will definitely need to useShared
to be consistent withArrayPool<T>.Shared
, so the other two would need to be changed in that case. Otherwise, we could just leave the names different in those cases, looking for feedbacks on this 😄Additional notes
A decision on this should be made before the 7.0 release is published (including #3230), and I was thinking maybe it'd be best to decide on this before #3424 is merged so that the updated name would already be included in the next preview package for the MVVM Toolkit, so that devs would have more time to get used to that?
Note regarding switching to
Shared
for messenger, that'd be different fromDefault
being used inMvvmLight
, but not necessarily an issue since devs moving from there would already have to make some code changes anyway.cc. @michael-hawker @jamesmcroft
The text was updated successfully, but these errors were encountered: