Skip to content

Fix Arg.all() after #30 + add more tests + small refactor - #43

Merged
ffMathy merged 1 commit into
ffMathy:masterfrom
Karql:fix-after-30
Apr 29, 2019
Merged

Fix Arg.all() after #30 + add more tests + small refactor#43
ffMathy merged 1 commit into
ffMathy:masterfrom
Karql:fix-after-30

Conversation

@Karql

@Karql Karql commented Apr 24, 2019

Copy link
Copy Markdown
Contributor

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

ffMathy commented Apr 25, 2019

Copy link
Copy Markdown
Owner

Great work! I made a single comment.

@Karql

Karql commented Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

@ffMathy I don't see any comments

Comment thread spec/index.test.ts
@ffMathy

ffMathy commented Apr 29, 2019

Copy link
Copy Markdown
Owner

Oh wow, forgot to hit "submit review".

@ffMathy

ffMathy commented Apr 29, 2019

Copy link
Copy Markdown
Owner

Thank you so much for this!

@ffMathy
ffMathy merged commit c64858e into ffMathy:master Apr 29, 2019
@Karql

Karql commented Apr 29, 2019

Copy link
Copy Markdown
Contributor Author

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

@ffMathy

ffMathy commented Apr 29, 2019

Copy link
Copy Markdown
Owner

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