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

update HACKING.pod #1064

Merged
merged 1 commit into from
Jun 5, 2019
Merged

update HACKING.pod #1064

merged 1 commit into from
Jun 5, 2019

Conversation

cgzones
Copy link
Contributor

@cgzones cgzones commented Aug 27, 2018

This is just a first draft. Please discuss.
Related: #1014, #1051, #1060, #1061


NAME, SYNOPSIS, DESCRIPTION, REQUIRED ARGUMENTS, OPTIONS, EXIT STATUS,
CONFIGURATION, FILES, VERSION, BUGS AND LIMITATIONS, AUTHOR,
LICENSE AND COPYRIGHT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are just suggestions, subject to change


B<perlcritic> is used to comply to some wide-spread recommendations. The exact
configuration can be seen in the file I<.perlcriticrc>. Please try to comply
with those rules by running c<make lint>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #1051 for questions about perlcritic severity 4 warnings


NAME, SYNOPSIS, DESCRIPTION, SUBROUTINES/METHODS,
CONFIGURATION AND ENVIRONMENT, DEPENDENCIES, BUGS AND LIMITATIONS, AUTHOR,
LICENSE AND COPYRIGHT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are just suggestions, subject to change

For plugins the recommend headers are (in this order):

NAME, APPLICABLE SYSTEMS, CONFIGURATION, USAGE, MAGIC MARKERS, BUGS, AUTHOR,
LICENSE AND COPYRIGHT
Copy link
Contributor Author

Choose a reason for hiding this comment

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

those are just suggestions, subject to change

HACKING.pod Outdated
We use B<perltidy> to automatically format perl code. This ensure a consistent
style and a tool-based enforcement. The exact configuration can be seen in the
file I<.perltidyrc>. Please try to use it before any contribution by running
C<make apply-formatting>.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

see #1014

@coveralls
Copy link

coveralls commented Aug 27, 2018

Pull Request Test Coverage Report for Build 2486

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 62.546%

Totals Coverage Status
Change from base Build 2485: 0.0%
Covered Lines: 1022
Relevant Lines: 1634

💛 - Coveralls

@sumpfralle
Copy link
Collaborator

Great work!

I think, you can omit the link to https://munin-monitoring.org/wiki - the guide should be sufficient.
Regarding the usual documentation headers I see an overlap between AUTHOR and LICENSE AND COPYRIGHT, but I have no strong opinions about that.
Thank you!

Copy link
Collaborator

@sumpfralle sumpfralle left a comment

Choose a reason for hiding this comment

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

That looks good to me!

Let us just wait for our current definition of the branches, before we merge it.

HACKING.pod Outdated
=head2 GITHUB

The main development of Munin is happening on L<https://github.com>. With a
free account you can report feature requests and issues
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nitpicking: I would not like to the see the word free attached to a proprietary service.

How about "After registering an account"?

HACKING.pod Outdated
=item I<stable-2.0>

As the name suggests this is the stable branch for versions 2.0.x.
Please base your plugin and bug-fix patches on this branch.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Indeed I usually advertised this policy (plugin improvements go to stable-2.0). But @steveschnepp and me just realized, that we have diverging opinions on that matter. I hope, we will find the time to discuss this tomorrow in the weekly meeting. Thus the final text of this paragraph will need to be decided in IRC and not in this pull request, I guess. (just my opinion - we will see)

I always had a bad feeling about telling people to do some rebase without being able to point to a written documentation detailing this. Thank you for reducing this pain in the future!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Update of our discussion: during the IRC meeting on the 5th of September we agreed on only applying fixes (both for core and for plugins) to stable-2.0 from now own. These changes will be merged into master from time to time. All other changes go directly to master.

HACKING.pod Outdated

=item C<Braces>
We prefer indentation via spaces. Tab-based indentation is usually relying on
Copy link
Member

Choose a reason for hiding this comment

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

Err... no ;-)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no? but most of the code is using spaces

Copy link
Member

Choose a reason for hiding this comment

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

Well, you have a point : it's about 65% of spaces and 35% of tabs.

~/src/munin$ grep -R '^ ' script/ lib/ plugins/ | wc -l                                                                                                                                                             
26066
~/src/munin$ grep -R '^        ' script/ lib/ plugins/ | wc -l                                                                                                                                                      
14301

Yet most of the new code is using tabs, as I wrote it. But I agree, the codebase is huge, and the ratio was very similar in 2.0. (70% spaces & 30% tabs)

@sumpfralle
Copy link
Collaborator

I think, there are two open issues:

  1. @cgzones: would you please phrase the usage of the two branches following our IRC discussion?
  2. @steveschnepp: how about the tab/space issue? I think, we all can live with every approach. Even something like follow the overall style of the file in question would be sufficient. But we should write something about it :)

@cgzones
Copy link
Contributor Author

cgzones commented Sep 25, 2018

updated the branch and tab/space section

there are still some TODO's left...

@steveschnepp steveschnepp merged commit 0ab1ed2 into munin-monitoring:master Jun 5, 2019
@steveschnepp
Copy link
Member

Let's merge it

@cgzones cgzones deleted the hacking branch July 15, 2020 16:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants