-
Notifications
You must be signed in to change notification settings - Fork 473
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
update HACKING.pod #1064
Conversation
|
||
NAME, SYNOPSIS, DESCRIPTION, REQUIRED ARGUMENTS, OPTIONS, EXIT STATUS, | ||
CONFIGURATION, FILES, VERSION, BUGS AND LIMITATIONS, AUTHOR, | ||
LICENSE AND COPYRIGHT |
There was a problem hiding this comment.
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>. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #1014
Pull Request Test Coverage Report for Build 2486
💛 - Coveralls |
Great work! I think, you can omit the link to https://munin-monitoring.org/wiki - the guide should be sufficient. |
There was a problem hiding this 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 |
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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!
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Err... no ;-)
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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)
I think, there are two open issues:
|
updated the branch and tab/space section there are still some TODO's left... |
Let's merge it |
This is just a first draft. Please discuss.
Related: #1014, #1051, #1060, #1061