TheKevJames / puppet-homebrew

homebrew (+brewcask! +taps!) package installer and provider
https://forge.puppet.com/thekevjames/homebrew
Apache License 2.0
18 stars 44 forks source link

Refactoring #50

Closed jordigg closed 8 years ago

jordigg commented 8 years ago
jordigg commented 8 years ago

Just tried on a fresh system and I still need to fix a couple things about the brew output and it's errors

jordigg commented 8 years ago

Found a fix, was easier than I tought... Running the commands with 2> /dev/null doesn't work with the existing code since it gets confused and uses 2> /dev/null as the name of the package. Running the command straight in the command line works tough.

By turning the combine option to false on the super function enables separation of returned outputs. This way we only get back what we want, in this case stdout. Super docs. This works fine on my test environment.

TheKevJames commented 8 years ago

Looks like travis tests didn't run for your PR. That should be fixed now for all future commits.

TheKevJames commented 8 years ago

Also, for future reference, please avoid mixing so many different changes in one PR. I'd be much happier to see separate PRs for: doc changes, params inheritance, stdout/stderr parsing changes, and string boolean consumption change. Separating these would also allow me to review and merge them easier and faster. No worries for this patch, but please keep in mind for the future!

jordigg commented 8 years ago

All my changes are getting mixed on git, it's becoming a mess. Maybe is better if I close this one and submit different PR's for each change like you said. Sorry so much for the trouble.