-
-
Notifications
You must be signed in to change notification settings - Fork 136
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
Changes from comment #47
Changes from comment #47
Conversation
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.
First off thanks for this PR, this is awesome :) Made a few comments on it :)
MvvmHelpers/WeakEventManager.cs
Outdated
namespace MvvmHelpers | ||
{ | ||
public class WeakEventManager | ||
internal class WeakEventManager |
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 reason we shouldn't make this public? Seems like it could be useful for library creators.
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 read the comment now... hmmmmmm interesting....
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.
Yes, actually that makes a lot of sense because I want to use the simplest solution in my apps, and I hope that's the case of a lot of other developers too and it's an extra piece of code that needs to be maintained and needs improvement over time.
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 think I am fine keeping it public :)
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.
Okay I will change it back to public
fixes: #45
Changes: