Skip to content

Conversation

@gmponos
Copy link
Contributor

@gmponos gmponos commented Dec 28, 2019

before merging #529 if we could merge this first..

@gmponos
Copy link
Contributor Author

gmponos commented Dec 28, 2019

@sagikazarmark
Copy link
Collaborator

sagikazarmark commented Dec 28, 2019

Why replace mocks with actual implementations? I mean, I understand the usual reasons, but in this case mocks seem acceptable to me.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 28, 2019

Why replace mocks with actual implementations?

For the usual reasons.. hahaah

  • I have seen cases in the past where there were many WTF moments because of mocks... For instance say that it throws an exception although it never does.. for the sake of good programming practices I switched here to actual implementation.
  • Although speed is not a problem here.. Prophesize is way heavier.

Let's not make a big deal out of it.. if this is no good I will revert.. no problem.

@sagikazarmark
Copy link
Collaborator

Even if we consider replacing mocks with actual implementations, that should be a separate PR, probably covering the whole library.

@gmponos
Copy link
Contributor Author

gmponos commented Dec 30, 2019

Revert the test changes about prophesize :)

Copy link
Collaborator

@sagikazarmark sagikazarmark left a comment

Choose a reason for hiding this comment

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

Thanks!

@sagikazarmark sagikazarmark merged commit a44e2f2 into moneyphp:master Mar 16, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants