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

Docker - Add examples to readme of using a dockersed image of shfmt. #121

Closed
wants to merge 2 commits into from
Closed

Conversation

jamesmstone
Copy link
Contributor

I made a docker image of shfmt, so I thought I would share it. If you would like I would be happy to make this more official by putting it as part of your repo.

I made a docker image of `shfmt`, so I thought I would share it. If you would like I would be happy to make this more official by putting it as part of your repo.
@mvdan
Copy link
Owner

mvdan commented Jun 10, 2017

Thanks for your contribution!

Adding this to the related projects section seems like a great idea. Adding it at the top of the README not so much, especially given how this is not an official image.

@mmlb raised a similar proposal in #68, but we decided against official Docker images for now.

@mmlb
Copy link

mmlb commented Jun 10, 2017

Right, @mvdan I've been thinking about adding -d support to shfmt, would you accept that? Missing -d is what what basically killed shfmt image for my use.

@mvdan
Copy link
Owner

mvdan commented Jun 10, 2017

Could you open a separate issue about that? With a proposal on how it would be implemented, in particular.

@jamesmstone
Copy link
Contributor Author

jamesmstone commented Jun 11, 2017

cheers have now limited it to just the related projects sections :)

@@ -87,6 +87,7 @@ the parser and the printer. To get started, run:

* [format-shell] - Atom plugin for `shfmt`
* [shell-format] - VS Code plugin for `shfmt`
* [dockerised-shfmt] - A docker image of `shfmt` **Example:** `docker run -it --rm -v "$(pwd)":/sh -w /sh jamesmstone/shfmt -l -w script.sh`
Copy link
Owner

Choose a reason for hiding this comment

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

Please remove the example too - the info should be in the page the link points to. Otherwise the README would be a wall of text :)

Copy link
Owner

@mvdan mvdan left a comment

Choose a reason for hiding this comment

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

Forgot to mark the PR as needing changes.

@mvdan mvdan closed this in 8d26992 Jul 3, 2017
@mvdan
Copy link
Owner

mvdan commented Jul 3, 2017

I've taken the liberty of cleaning up the patch and applying it to master, keeping you as the author. Thanks again!

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.

3 participants