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

puppet-lint: Enable whitespace checker #947

Closed
wants to merge 1 commit into from

Conversation

bastelfreak
Copy link
Member

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

@bastelfreak bastelfreak added the enhancement New feature or request label Jun 23, 2020
@bastelfreak bastelfreak self-assigned this Jun 23, 2020
@bastelfreak
Copy link
Member Author

Hi people,
please comment on the diff. I would like to add https://github.com/kuleuven/puppet-lint-manifest_whitespace-check to our testmatrix. The whole diff is caused by the autofix. I'm currently tracking down one bug: voxpupuli/puppet-lint-manifest_whitespace-check#3

@bastelfreak
Copy link
Member Author

@@ -2,11 +2,10 @@
class collectd::plugin::apache (
Enum['present', 'absent'] $ensure = 'present',
Boolean $manage_package = $collectd::manage_package,
Hash $instances = { 'localhost' => { 'url' => 'http://localhost/mod_status?auto' } },
Hash $instances = { 'localhost' => { 'url' => 'http://localhost/mod_status?auto' }},
Copy link
Member

Choose a reason for hiding this comment

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

Why is this space removed, but not the others?

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bug. It should keep the space.

Copy link
Contributor

Choose a reason for hiding this comment

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

As a side note, I had a similar case where I had a default value for a Hash (but more data and thus multiline), which crashed class_parameter-check

@@ -33,12 +32,12 @@

case $facts['kernel'] {
'OpenBSD': { $has_wordexp = false }
default: { $has_wordexp = true }
default: { $has_wordexp = true }
Copy link
Member

Choose a reason for hiding this comment

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

It made sense to align the cases before though?

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

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

Looks good.

Comment on lines +14 to +15
gem 'voxpupuli-test', '>= 1.4.0', :require => false
gem 'puppet-lint-manifest_whitespace-check', :require => false
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to make this a part of vp-test 1.5.0 if it works well (which it appears to do)

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, me too. I just added it for testing / enable people to reproduce this.

@vox-pupuli-tasks
Copy link

Dear @bastelfreak, thanks for the PR!

This is Vox Pupuli Tasks, your friendly Vox Pupuli GitHub Bot. I noticed that your pull request contains merge conflict. Can you please rebase?

You can find my sourcecode at voxpupuli/vox-pupuli-tasks

@ekohl
Copy link
Member

ekohl commented Aug 30, 2020

This was included in modulesync 3.0.0

@ekohl ekohl closed this Aug 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request merge-conflicts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants