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

Add EditorConfig #428

Merged
merged 2 commits into from May 10, 2021
Merged

Conversation

jeffbyrnes
Copy link
Contributor

A helpful way to adhere to a common set of whitespace concerns.

https://editorconfig.org

@jeffbyrnes jeffbyrnes requested review from a team as code owners April 16, 2021 20:05
@CLAassistant
Copy link

CLAassistant commented Apr 16, 2021

CLA assistant check
All committers have signed the CLA.

@sanfrancrisko
Copy link
Contributor

Thanks for the contribution @jeffbyrnes . There may be some issues merging this PR, given that it is enforcing code style with less / no interaction from the users. E.g. at the moment, a user could run pdk validate and fix violations reported back from Rubocop themselves. Not being massively familiar with how the most popular IDEs interact with an EditorConfig config file, that also makes me cautious about potential negative user experiences if this were to appear on the next pdk update.

The conversation about introducing markdown linting / validation has come up too in puppetlabs/pdk#1051 and we've yet to define what the MD linting spec would be.

We would potentially be willing to get this over the line if we were to be set to umanaged: true by default in the .sync.yml, so it was opt in for this config. I do think it it is a positive contribution, but, one that could potentially disrupt some folks.

Happy to be talked in to it, if you feel strongly 😃

@jpogran , @da-ar , @binford2k - any thoughts?

@jeffbyrnes
Copy link
Contributor Author

jeffbyrnes commented Apr 19, 2021

@sanfrancrisko I hear you on all points, and this can be changed to suit whatever you feel is appropriate.

I will say that, without actually having EditorConfig installed as part of your $EDITOR, this file will have no effect, so folks who are uninterested in using it will not be impacted.

For those with built-in or plugin-provided EditorConfig, it indeed is possible that it may have impact for someone where previously there was none.

The Markdown whitespace rules follow the “official” guidelines from its creator, John Gruber:

  • 4-space indents
  • Two spaces at the end of a line is meaningful (represents a line break), so do not strip trailing whitespace

The Makefile rules are there b/c make is sensitive to tabs vs spaces, and it’s bitten me more than once. But not really necessary, since lots of folks just use rake.

As for the base rules, those are built on the Ruby community conventions, and since Puppet is built on top of Ruby, it’s always made sense to me to treat it the same.

I think using unmanaged: true as the default is totally fine! My biggest encouragement is that whitespace shouldn’t be something inconsistent across a project or team, and this helps with that while avoiding personal conflicts (does away with nitpicking in PRs & such; EditorConfig enforces the rules as soon as you save a file).

@sanfrancrisko
Copy link
Contributor

Hi @jeffbyrnes , thanks for coming back so rapidly and apologies for the delay my side.

If you were happy to transform this in to an .erb template (albeit, without any parameters at the moment) and set it to unmanaged: true in the config_defaults.yml then we'd be happy to get this over the line. Give me a shout if you need any help.

Thanks again for the contribution 👍

A helpful way to adhere to a common set of whitespace concerns.

https://editorconfig.org
* YAML syntax highlighting is nice
* CLI commands are generally better shown as a code block
@jeffbyrnes
Copy link
Contributor Author

@sanfrancrisko sure thing! I’ve updated it to be an unmanaged by default template, and added docs to the README detailing how to use it.

While in the README, I also made some tweaks in eb1b372, if those are unwanted, simply say the word & they’re gone.

Copy link
Contributor

@sanfrancrisko sanfrancrisko left a comment

Choose a reason for hiding this comment

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

Sorry for the delay in getting to you @jeffbyrnes. This LGTM!

Thanks for incorporating the feedback from the team and the additional clean up, much appreciated 👍

@sanfrancrisko sanfrancrisko merged commit 377df33 into puppetlabs:main May 10, 2021
@jeffbyrnes jeffbyrnes deleted the add-editorconfig branch May 10, 2021 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants