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

Fix issue #27 #31

Merged
merged 1 commit into from
Oct 19, 2020
Merged

Fix issue #27 #31

merged 1 commit into from
Oct 19, 2020

Conversation

GuilleEneas
Copy link
Contributor

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
Copy link

Similar files are

2 similar comments
@similar-code-searcher
Copy link

Similar files are

@similar-code-searcher
Copy link

Similar files are

@GuilleEneas GuilleEneas changed the title Fix issue 27 Fix issue #27 Oct 15, 2020
@shairez
Copy link
Member

shairez commented Oct 16, 2020

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

Please checkout my comments and let me know what you think

@GuilleEneas
Copy link
Contributor Author

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
Copy link
Member

shairez commented Oct 18, 2020

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!

…s configured

when spy configured with mustBeCalledWith and the method isn't called with the expected parameters,
the method name will be displayed in the logs

fix #27
@GuilleEneas
Copy link
Contributor Author

@shairez, changes implemented 😉

@shairez
Copy link
Member

shairez commented Oct 19, 2020

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
Copy link
Member

shairez commented Oct 19, 2020

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

@allcontributors
Copy link
Contributor

@shairez

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

@shairez shairez merged commit 281e128 into hirezio:fix-issue-27 Oct 19, 2020
@shairez
Copy link
Member

shairez commented Oct 19, 2020

MERGED! 🎉😀

Thanks again for your contribution 🙏
Great job! 💪

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