Skip to content

Conversation

@Enrice
Copy link
Contributor

@Enrice Enrice commented Oct 21, 2025

Pull Request (PR) description

All parameters with feasible defaults in nginx itself become Optional.
Also fixes some minor type isssues.

This Pull Request (PR) fixes the following issues

Fixes #1660

@Enrice Enrice marked this pull request as draft October 21, 2025 18:19
@kenyon
Copy link
Member

kenyon commented Oct 21, 2025

Where you're making changes to every parameter, maybe also stop aligning the equals signs (just have one space of separation, like Integer $abc = 0,) so that we don't have so many diffs due to alignment changes later.

@Enrice
Copy link
Contributor Author

Enrice commented Oct 22, 2025

Do you mean only simple assignments or also parameter defaults?

For my understanding see latest commit.

@Enrice Enrice marked this pull request as ready for review October 22, 2025 12:04
@Enrice Enrice marked this pull request as draft October 22, 2025 12:09
@Enrice Enrice marked this pull request as ready for review October 22, 2025 14:33
@kenyon
Copy link
Member

kenyon commented Oct 22, 2025

Do you mean only simple assignments or also parameter defaults?

For my understanding see latest commit.

For all class parameter lists, I think we shouldn't be aligning the equals signs. It just causes too many diffs when there are changes to the parameter lists.

@Enrice
Copy link
Contributor Author

Enrice commented Oct 23, 2025

puppet style guide says otherwise:
https://help.puppet.com/core//8/Content/PuppetCore/style_guide_classes.htm#Parameterindentationandalignment

(You should however not take the examples too serious as they are displayed, there seems to be a HTML issue. So inspect the HTML source instead...)

That taken into account the current state of the PR should be ok...

@kenyon
Copy link
Member

kenyon commented Oct 23, 2025

That part of the style guide I don't think we should follow, especially for a module like this with large lists of parameters. See also voxpupuli/voxpupuli.github.io#322

@Enrice
Copy link
Contributor Author

Enrice commented Oct 27, 2025

ok, then everything should be fine now.

@kenyon kenyon added the backwards-incompatible This change will lead to a major version bump for the next release label Oct 27, 2025
Copy link
Member

@kenyon kenyon left a comment

Choose a reason for hiding this comment

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

Minor thing, but not sure it's valuable to have nginx defaults in the comments, since the defaults can change.

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

Labels

backwards-incompatible This change will lead to a major version bump for the next release

Projects

None yet

Development

Successfully merging this pull request may close these issues.

unnecessary explicit defaults

2 participants