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

(MODULES-8088) - newline_spec.rb test expectation update #531

Merged
merged 1 commit into from
Oct 16, 2018

Conversation

lionce
Copy link
Contributor

@lionce lionce commented Oct 15, 2018

This commit updates the expected test output. The following feature (71e55c4) updates the test output however this is not necessary and caused failures.

Copy link
Contributor

@glennsarti glennsarti left a comment

Choose a reason for hiding this comment

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

The commit message needs some work as it doesn't really comply with the module Contributor Guidelines

While I can see the code, there are no links or references to understand why this commit should be merged.

@lionce lionce changed the title update newline spec (MODULES-8088)-update newline spec Oct 16, 2018
@lionce lionce changed the title (MODULES-8088)-update newline spec (MODULES-8088)- newline_spec.rb test fix Oct 16, 2018
@pmcmaw pmcmaw added bugfix and removed maintenance labels Oct 16, 2018
@david22swan
Copy link
Member

@glennsarti Are you happy with the changes?

@david22swan
Copy link
Member

Adhoc Pipeline Passed

Copy link
Contributor

@tkishel tkishel left a comment

Choose a reason for hiding this comment

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

I would not have expected that a change in the code would not require a symmetrical change in the test, but that may be outside the scope of this pull request :) Thanks for taking the time to help with this. It's a small change, but important to at least one user.

@glennsarti
Copy link
Contributor

@david22swan Honestly, not really, however I am not a maintainer of this module, so if you're ok with it, so be it.

From Contributing Guide

The body of my commit message:

  • is meaningful
  • uses the imperative, present tense: "change", not "changed" or "changes"
  • includes motivation for the change, and contrasts its implementation with the previous behavior

@glennsarti glennsarti dismissed their stale review October 16, 2018 14:16

My review is not a requirement

@david22swan david22swan changed the title (MODULES-8088)- newline_spec.rb test fix (MODULES-8088) - newline_spec.rb test expectation update Oct 16, 2018
@pmcmaw
Copy link
Contributor

pmcmaw commented Oct 16, 2018

Thanks @glennsarti, the title has been updated. We are going to merge this PR as it is blocking puppet-agent. However we plan in the upcoming days on scheduling a session regarding all this. So better commit messages etc are on their way! 👍

@david22swan david22swan merged commit e74f378 into puppetlabs:master Oct 16, 2018
@pmcmaw
Copy link
Contributor

pmcmaw commented Oct 16, 2018

Hey @ekinanp just to let you know this change has been merged.
Apologies for the delay on this! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants