Skip to content
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

Do not access Global via service locator in MainWindowViewModel c… #3756

Conversation

kiminuo
Copy link
Collaborator

@kiminuo kiminuo commented Jun 10, 2020

…lass

Copy link
Collaborator

@molnard molnard left a comment

Choose a reason for hiding this comment

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

Very well done. Much cleaner!

@nopara73
Copy link
Contributor

We're going back and forth with this. I remember like 3 times this was flipped and flopped: we started out with statics, then @NicolasDorier added them to constructors instead, then @danwalmsley, @molnard and me removed these from the constructor. I don't mind any of the two approaches, it's just let's settle with a single one. I suggest @danwalmsley to have the final say.

@nopara73 nopara73 removed their request for review June 11, 2020 10:25
@NicolasDorier
Copy link
Collaborator

NicolasDorier commented Jun 11, 2020

I removed every static in the hope of making unit tests where you could simulate several wasabi wallets at once via their view model.

Those unit test PR never got merged. If there is a single static singleton, then interactions between several wasabi wallet can't be tested, thus defeating the whole purpose of the exercise.

@danwalmsley reason to reintroduce singleton was meant to be temporary if I remember.

Copy link
Collaborator

@lontivero lontivero left a comment

Choose a reason for hiding this comment

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

This is better than what we have.

@lontivero lontivero merged commit a137cac into zkSNACKs:master Jun 12, 2020
@danwalmsley
Copy link
Collaborator

We're going back and forth with this. I remember like 3 times this was flipped and flopped: we started out with statics, then @NicolasDorier added them to constructors instead, then @danwalmsley, @molnard and me removed these from the constructor. I don't mind any of the two approaches, it's just let's settle with a single one. I suggest @danwalmsley to have the final say.

Offline in meetings and discussions with @kiminuo @lontivero @molnard the direction we would like to go in is:

  1. interfaces passed to the viewmodels representing only the dependencies required by that viewmodel.
  2. no statics / globals

@molnard
Copy link
Collaborator

molnard commented Jul 1, 2020

We're going back and forth with this. I remember like 3 times this was flipped and flopped: we started out with statics, then @NicolasDorier added them to constructors instead, then @danwalmsley, @molnard and me removed these from the constructor. I don't mind any of the two approaches, it's just let's settle with a single one. I suggest @danwalmsley to have the final say.

Offline in meetings and discussions with @kiminuo @lontivero @molnard the direction we would like to go in is:

  1. interfaces passed to the viewmodels representing only the dependencies required by that viewmodel.

Interfaces or objects but yes.

  1. no statics / globals

Agree, statics can be allowed in some cases but only if make sense to use them.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants