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

Add optional output argument to mustache bin #540

Merged
merged 3 commits into from
Jan 14, 2016
Merged

Add optional output argument to mustache bin #540

merged 3 commits into from
Jan 14, 2016

Conversation

wizawu
Copy link
Contributor

@wizawu wizawu commented Jan 10, 2016

I am not sure whether this PR is welcome, since I didn't see much people had a requirement for using this argument.
Yet we have been using this feature in production (because > output is forbidden in certain cases).
Feel free to close it if you find it confusing.
Thanks.

@wizawu wizawu changed the title Add output argument to mustache bin Add optional output argument to mustache bin Jan 10, 2016
@phillipj
Copy link
Collaborator

Seems like a good feature 👍 Could you please add atleast one test for it aswell?

@wizawu
Copy link
Contributor Author

wizawu commented Jan 10, 2016

Sorry, test added now.
I think probably I shouldn't update the README. It might cause a mess 😞

@@ -0,0 +1 @@
Howdy LeBron, CLI rox
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking this file shouldn't be commited, it would even be better if it were deleted after the tests has run.

When this file exists in the repo, your changes in ./bin/mustache could actually be reverted, and the test below would still pass - right?

@phillipj
Copy link
Collaborator

Sorry, test added now.

Nice! One little comment about the output file, otherwise it LGTM.

@wizawu
Copy link
Contributor Author

wizawu commented Jan 14, 2016

Done 💪

phillipj added a commit that referenced this pull request Jan 14, 2016
Add optional `output` argument to mustache bin
@phillipj phillipj merged commit e1997ee into janl:master Jan 14, 2016
@phillipj
Copy link
Collaborator

Thanks! 👍

@dasilvacontin
Copy link
Collaborator

Btw @wizawu, what's the case in which > output is forbidden? I'm curious. :)

@wizawu
Copy link
Contributor Author

wizawu commented Mar 15, 2016

When using ansible's shell module.

- name: append
  shell: mustache .... >> output

- name: there won't be output
  shell: mustache .... > output

Apparently we don't want to append to output.

@dasilvacontin
Copy link
Collaborator

@wizawu But does it raise an error, or?

Have you checked ansible/ansible#9791 and/or http://docs.ansible.com/ansible/shell_module.html, specially:

# Run a command that uses non-posix shell-isms (in this example /bin/sh
# doesn't handle redirection and wildcards together but bash does)
- shell: cat < /tmp/*txt
  args:
    executable: /bin/bash

@wizawu
Copy link
Contributor Author

wizawu commented Mar 16, 2016

It won't raise an error. It would exit normally, but without any output. I know it is not a bug of mustache.

And yes, I've checked those docs. Thanks :) I just thought adding a third argument would be easier for me. I do agree that it is just an uncommon scene.

There is another case in package.json

{
  "scripts": {
    "build:html": "mustache view.json",
    "build:html:all": "ls html | xargs -I {} npm run build:html {} > dist/{}",
  }
}

The second script won't work as expected, since > dist/{} won't be concated to build:html.

I also agree that it is another uncommon scene.

@phillipj
Copy link
Collaborator

FYI if you'd like to send custom arguments to a npm script, like it seems you're trying to with build:html, you'll have to use double dash before any of the arguments arguments. E.g. npm run build:html -- page.html

Read more about it in npm run script docs.

@wizawu
Copy link
Contributor Author

wizawu commented Mar 22, 2016

@phillipj Very useful!

But after all, > won't be treated as an argument.

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.

4 participants