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

Array support #7

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

JoeBartelmo
Copy link
Contributor

So this was a little convoluted, and I think this is an appropriate fix.

Essentially the issue is primarily with the way this module is calling the chimp executable. According to the documentation located here tags need to be passed in 2 ways. If we want to AND tags together, we need to have
--tag=@tagname --tag=@tagname2 where as if we want to OR: --tag=@tagname,@tagname2

This chimp module grabs the pre-formatted arguments. So if we pass it an array, the translation is the following: tags: ['@tag1', '@tag2'] ~> '@tag1,@tag2' which isn't want we want all the time. So this fix allows you to pass an array of tags to support so if you want @tag1 AND @tag2 OR @tag3 we would pass: tags: JSON.stringify(['@tag1', '@tag2,@tag3'])

This does not break any functional code, so if people pass in tags: '@tag1,tag2' it will work fine.

I have changed your test grunt file to accomidate and demonstrate these changes, i recommend you start the code review there.
image

@JoeBartelmo
Copy link
Contributor Author

@btburton42

@btburton42
Copy link
Owner

@JoeBartelmo what version of node are you using? I'm on 8.1.2 and it's failing to run tests due to what I believe to be a bug in the Chimp lib.

screenshot 2017-06-29 10 18 16

@JoeBartelmo
Copy link
Contributor Author

@btburton42 Sorry I didn't see your message. I'm on the current LTS version of node

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