hirezio / auto-spies

Create automatic spies from classes
MIT License
181 stars 30 forks source link

Fix issue #27 #31

Closed GuilleEneas closed 4 years ago

GuilleEneas commented 4 years ago

Hi Guys, I don't know what is the protocol here as the issues is already described and we discussed we were going to fix it quickly. Somehow I manage to fix it in all packages just touching the core, therefore only the tests were failing.

I also thought that maybe we could put the name of the method in a more contrasted way, therefore the users will see it better.

similar-code-searcher[bot] commented 4 years ago

Similar files are

similar-code-searcher[bot] commented 4 years ago

Similar files are

similar-code-searcher[bot] commented 4 years ago

Similar files are

shairez commented 4 years ago

Great job @GuilleEneas ! Thanks for this PR! 🙏💪

Please checkout my comments and let me know what you think

GuilleEneas commented 4 years ago

Thank you @shairez !!!

I had a lot of doubts when implementing in what do. Add this attribute in this type or... btw, first I made that attribute optional, but when calling the error handling method the linter told me that I couldn't use the !. It didn't make any sense (to me at least) to make the function name optional in the throwArgumentsError so I decided to do what I did in first place. After your comments I remove it from the object and add an extra parameter wherever I had to.

Also squeezed all commits into one to not pollute the git history 🙂

Please take a look to the final result and let me know if you want me to change anything else.

Looking forward to contribute more 🤓

shairez commented 4 years ago

Thanks for the fixes @GuilleEneas !

I've added 2 more comments, please check them out after those, I believe we're ready to merge it

Thanks again!

GuilleEneas commented 4 years ago

@shairez, changes implemented 😉

shairez commented 4 years ago

I will move it to the common describe, also in the version for jasmine. To me having a number feels a bit arbitrary, nonetheless I never know when in my tests should I extract something to a more global scope vs keeping things together where they belong.

It comes from here - https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming)

So it's not that arbitrary 😀

Here's a nice article about it - https://andrewbrookins.com/technology/the-rule-of-three/

Thanks again for making the fix!

shairez commented 4 years ago

@all-contributors please add @GuilleEneas for code and tests

allcontributors[bot] commented 4 years ago

@shairez

I've put up a pull request to add @GuilleEneas! :tada:

shairez commented 4 years ago

MERGED! 🎉😀

Thanks again for your contribution 🙏 Great job! 💪