search

voxpupuli / puppet-elasticsearch

Elasticsearch Puppet module
Apache License 2.0
404 stars 479 forks source link

Remove legacy top-scope syntax #1205

Closed smortex closed 4 months ago

kenyon commented 7 months ago

Just changes in CHANGELOG and HISTORY. Maybe we don't want to change these?

smortex commented 7 months ago

Just changes in CHANGELOG and HISTORY. Maybe we don't want to change these?

I am not 100% sure about it. As you probably guessed, I automated these fixes on all voxpupuli modules and opened draft-PRs to review them and have CI confirm I don't break everything :smile:

I started this effort because I saw that a coworker who is starting to use Puppet got really confused about these :: he saw in some places and not in other ones. So I try to make it easier for newcomers by having a more consistent syntax.

There are a few PRs like this one where only the "doc" (e.c. CHANGELOG) is updated. At first I though we can ignore these and close the PR, and then I though that keeping them was going against the idea of unification I was following… IMHO, fixing README.md is a must-do, so fixing CHANGELOG.md to match the syntax is probably fine.

This is of course open for discussion because I am also not excited by rewriting old changelog entries.

kenyon commented 7 months ago

Yes, agreed that it's a good idea to eliminate legacy syntax in as many places as possible so newcomers don't get confused. My only concern was that these changes could get undone by the changelog generator, but these ones don't look autogenerated, which is why I approved.