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

suggested edits on README #80

Merged
merged 1 commit into from
May 2, 2017
Merged

suggested edits on README #80

merged 1 commit into from
May 2, 2017

Conversation

jbondpdx
Copy link
Contributor

Howdy!

As part of the work for the Puppet SDK, I'd like to submit some edits to the Puppet::Syntax README. I've edited and reorganized the README with best docs practices in mind; of please let me know if I've got something wrong or you'd like me to make adjustments.

Pinging @DavidS for visibility (also, he helped review my changes before this PR).

Copy link
Contributor Author

@jbondpdx jbondpdx left a comment

Choose a reason for hiding this comment

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

I removed a line but reconsidered. :-)

README.md Outdated

Checks `.erb` files for syntax errors
The `app_management` setting is supported with Puppet 4.3 or greater and is off by default.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Add back in: With Puppet 5, app_management is always enabled.

Copy link
Contributor

@domcleal domcleal left a comment

Choose a reason for hiding this comment

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

The external (and private) ticket number should be removed from the commit message.

README.md Outdated

- Puppet >= 2.7 that provides the `validate` face.
- Ruby >= 1.8 with `erb` from stdlib.
- Ruby >= 1.8 with `erb` from `puppetlabs-stdlib`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ruby's stdlib, not puppetlabs-stdlib (isn't it?).

Copy link
Contributor

Choose a reason for hiding this comment

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

Errr. Yes. I missed that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oops, that makes sense, sorry!

README.md Outdated

If you are trying to validate the syntax of code written for application orchestration, you can enable the `app_management` setting:
task :test => [
Copy link
Contributor

Choose a reason for hiding this comment

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

Formatting here is missing, more indentation is required.

@jbondpdx
Copy link
Contributor Author

Thank you for the review, @domcleal . I've updated my PR with those fixes.

I squashed the commits, as that's usually what I do at work. But truth be told, I haven't contributed to community projects before, so I don't really know whether squashing them is proper etiquette or not. :-)

@jbondpdx jbondpdx changed the title (SDK-44) suggested edits on README suggested edits on README Apr 27, 2017
README.md Outdated

Test all manifests and templates relative to your `Rakefile`:
$ gem 'puppet-syntax'
Copy link
Contributor

Choose a reason for hiding this comment

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

The $ should be removed, it shouldn't be in a Gemfile and isn't a command.

README.md Outdated

A non-zero exit code and error message will be returned for any failures:
$ bundle
Copy link
Contributor

Choose a reason for hiding this comment

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

bundle install

@domcleal
Copy link
Contributor

Thanks for the update, a couple more comments that I missed the first time. Squashing the changes is fine.

@jbondpdx
Copy link
Contributor Author

Thank you, @domcleal , I've updated the PR.

@domcleal domcleal merged commit 4c9342a into voxpupuli:master May 2, 2017
@domcleal
Copy link
Contributor

domcleal commented May 2, 2017

Merged, thanks @jbondpdx.

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.

None yet

3 participants