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

Add configuration of command line args for puppet-lint #824

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
2 participants
@caldwell
Contributor

caldwell commented Dec 19, 2015

puppet-lint only allows configuration of its warnings through command line options. This small patch adds an option to the puppet-lint checker so I can configure the warnings to my liking.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Dec 20, 2015

Thank you for this patch, but I would rather prefer a specific option for warning control instead of a generic command line arguments option. Is that not possible?

@caldwell

This comment has been minimized.

Contributor

caldwell commented Dec 20, 2015

The documentation only mentions command line arguments or Rake options (not applicable here). There are no general warning names that do what you want.

For instance, the "2 Soft Space Tabs" page mentions what the warning checks for and says:

To disable this check you can add --no-2sp_soft_tabs-check to your puppet-lint command line.

You can kind of tell by the page URL that its internal name is 2sp_soft_tabs and that the command line option is built like --no-#{check_name}-check but it seems unreasonable to me to force everyone who wants to disable warnings to figure that out.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Dec 22, 2015

@caldwell Why would that be unreasonable? After all, the name of the check is mentioned right below the line you've quoted:

Alternatively, if you’re calling puppet-lint via the Rake task, you should insert the following line to your Rakefile.

PuppetLint.configuration.send('disable_2sp_soft_tabs')

Is the mental step from PuppetLint.configuration.send(…) to (add-to-list 'flycheck-puppetlint-disable-warnings …) that hard? Wouldn't the analogy between Flycheck options and Rake options be somewhat obvious even to the casual reader of the documentation?

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Dec 22, 2015

Besides, is there a reason why you don't use Control Comments? Back when I was still using puppet I exclusively used these for warning control, so that every co-worker and the CI would have the same idea of excluded warnings.

@caldwell

This comment has been minimized.

Contributor

caldwell commented Dec 22, 2015

Is the mental step from PuppetLint.configuration.send(…) to (add-to-list 'flycheck-puppetlint-disable-warnings …) that hard?

IMO, yes. It requires documentation on the flycheck side, where just taking command line args does not. If the puppet-lint docs called out the warning names explicitly rather than burying them in substrings I wouldn't think so.

Besides, is there a reason why you don't use Control Comments?

We currently don't run puppet-lint on our large puppet codebase, so there are now a ton of warnings, quite a lot of them being things we'll never change (80cols, eg.). So adding a bunch of control comments to every single file seems a bit much. It's really too bad they don't have a config file or something. But they don't, and so I want to turn off the egregious warnings just so my emacs looks reasonable and doesn't have every line marked.

@caldwell

This comment has been minimized.

Contributor

caldwell commented Dec 22, 2015

BTW, if you aren't going to budge on your stance I will change the code to do the (concat), though I'm fighting against it. In the end it's your codebase and I'd rather have the feature at least exist than to have to keep maintaining my own branch.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Dec 25, 2015

@caldwell Well, we document all our options anyway ☺️ so with a little help from the docstring it shouldn't be too hard to go from the Rake configuration to a Flycheck setting.

It's really too bad they don't have a config file or something.

Well, according to the README they have:

puppet-lint will also check for a .puppet-lint.rc file in the current directory and your home directory and read in flags from there, so if you wanted to always skip the hard tab character check, you could create ~/.puppet-lint.rc containing

--no-hard_tabs-check

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jan 2, 2016

@caldwell I'm sorry, did you miss my comment?

@caldwell

This comment has been minimized.

Contributor

caldwell commented Jan 3, 2016

Sorry, didn't miss it, just got busy during the holidays and forgot I needed to reply.

Wow, I somehow missed that .puppet-lint.rc file (I even poked through the source because I thought they must have something like that).

At any rate, that config does do what I want, so if you'd like to close this, that is ok with me.

If you'd like me to tweak the patch so it uses a list of warning names instead of the full command line option, I can do that too.

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jan 3, 2016

Sorry, didn't miss it, just got busy during the holidays and forgot I needed to reply.

Never mind. I hope you had nice holidays, and a happy new year!

If you'd like me to tweak the patch so it uses a list of warning names instead of the full command line option, I can do that too.

That'd be awesome 😍

@caldwell caldwell force-pushed the caldwell:puppet-lint-args branch from 6e35705 to aed1ae5 Jan 3, 2016

@caldwell

This comment has been minimized.

Contributor

caldwell commented Jan 3, 2016

Ok, I amended my branch to take the names instead of the entire args and added some (very) light documentation.

The only weird thing is I ended up using (option-list "")—I needed to have a filter function anyway to put the "-check" on the end of arg and I liked having the prefix, middle, and suffix all in one place.

@lunaryorn lunaryorn self-assigned this Jan 7, 2016

@lunaryorn lunaryorn closed this in 538aabe Jan 9, 2016

@lunaryorn lunaryorn removed the S-ready label Jan 9, 2016

@lunaryorn

This comment has been minimized.

Contributor

lunaryorn commented Jan 9, 2016

@caldwell Thank you :) I cherry-picked your changes to master ☺️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment