Skip to content

Conversation

@Karql
Copy link
Contributor

@Karql Karql commented Apr 24, 2019

Hi @ffMathy ;)

Today I've noticed that after #30 Arg.all() stops working when number of arguments is greater than 1.

--for(var i=0;i<Math.min(b.length, a.length);i++) {
++for(var i=0;i<Math.max(b.length, a.length);i++) {

For e.g. [Arg.all()] and [1,2] second iteration returns false.
With min is also not correct because [1,2] and [1,2,3] return true.

I've added

    if (a.find(x => x instanceof AllArguments) || b.find(b => b instanceof AllArguments)) {
        return true;
    }

in areArgumentArraysEqual

Rest changes:

  • more tests
  • small refactor

I hope you will enjoy it ;)

Regards!

CC: @domasx2

@Karql Karql changed the title Fix Arg.all() after #31 + add more tests + small refactor Fix Arg.all() after #30 + add more tests + small refactor Apr 24, 2019
@ffMathy
Copy link
Owner

ffMathy commented Apr 25, 2019

Great work! I made a single comment.

@Karql
Copy link
Contributor Author

Karql commented Apr 29, 2019

@ffMathy I don't see any comments

@ffMathy
Copy link
Owner

ffMathy commented Apr 29, 2019

Oh wow, forgot to hit "submit review".

@ffMathy
Copy link
Owner

ffMathy commented Apr 29, 2019

Thank you so much for this!

@ffMathy ffMathy merged commit c64858e into ffMathy:master Apr 29, 2019
@Karql
Copy link
Contributor Author

Karql commented Apr 29, 2019

No problem!
I like this lib so I'll try to help make it event better ;)

@ffMathy
Copy link
Owner

ffMathy commented Apr 29, 2019

Very cool! It's very appreciated. Perhaps I can put you as an official collaborator when I see some more PRs 👍

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