Add CONTRIBUTING.md file

Question

Add CONTRIBUTING.md file

elazar opened this issue 9 years ago · 15 comments

elazar commented 9 years ago

Answer 1 · 2015-05-25T17:09:17.000Z

We need a style guide!

Established:

indent = 4sp
methods, properties + variables = camelCase
K&R braces (should that include elseif/else on same line as closing brace?)
binary operators with constants - the constant comes to the right of the binary operator (eg. $var === null)

To be discussed:

array() vs [] - use one style in all cases, or different styles for different specific uses?
closure whitespace: function($params) { } or function ($params) { }? Should the closure code always be separated from its opening and closing braces by newlines, or can closures be declared on a single line?
Should there be a unified convention for naming custom events, or should plugin developers be free to name them at their discretion?
Indentation of chained method calls ($foo->setBar($bar)->setBaz($baz)->setQuux($quux);)
Should all setters be chainable as a convention?
Indentation of multi-line condition statements? Should the parentheses of multi-line condition statements be inline, or be proceeded/preceded by newlines? Should the operator be appended to the end of the previous line, or prepended to the front of the next line?

Answer 2 · 2015-05-25T18:16:16.000Z

Re: K&R braces, yes, that includes elseif / else.
Re: array() vs [], any current usage of array() stems from prior support for PHP 5.3, which didn't include support the shorter array syntax [] added in 5.4. Any repos still indicating 5.3 support in composer.json should be migrated to use 5.4.2 as a minimum version and any existing usage of array() in those repos should be replaced by [] for readability.
Re: closure whitespace, I've typically included no space between function and ( and included one space between use and (, but there's no particular reasoning for it other than personal preference and I'm open to discussion for a different convention to be used. On a related note, we should probably look into tools that can be used to indicate any coding standard issues as part of the Travis CI builds and/or potential adoption of PSR-1 and PSR-2.
Re: event naming, I've generally advised that event names follow the convention namespace.subnamespace.event using a root namespace equal or similar to the plugin emitting the events and as many subnamespaces as are needed / reasonable. Again, I'm open to discussion for cases this doesn't and should support.

Answer 3 · 2015-05-25T18:43:38.000Z

PSR-2#6 prescribes function ($args) use ($vars) {, which I personally prefer, rather than function($args) {.

With arrays, my own personal projects use array() for initialising and closing multi-line declarations (one key=>value pair per line, with commas, including the last pair), square braces for the empty array [] and single-line "list" array declarations, and avoid declaring single line associative arrays with key=>value pairs. I'm pretty sure this isn't very common, and I'm sure most would advocate using one syntax consistently.

Also, the empty array could be [], [ ], array() or array( ).

Oh, and we need an opinion on whether or not the double-arrow => should be aligned, and if so, how!

Answer 4 · 2015-05-25T19:11:42.000Z

PSR-2#6 prescribes function ($args) use ($vars) {, which I personally prefer, rather than function($args) {.

Since pursuing that would allow us to use any PSR-2-compatible tools, I'm OK with that argument. If we want to pursue PSR-2 adherence, I'd suggest filing another issue that covers any changes resulting from running an evaluation tool against all the current repos and linking that issue from here.

With arrays, my own personal projects use array() for initialising and closing multi-line declarations (one key=>value pair per line, with commas, including the last pair), square braces for the empty array [] and single-line "list" array declarations, and avoid declaring single line associative arrays with key=>value pairs. I'm pretty sure this isn't very common, and I'm sure most would advocate using one syntax consistently.

I'd agree consistency using [] is probably best, but otherwise, we're in agreement: one line per pair, single line is permissible if an assigned array only contains a single pair, and for multi-line assignments all lines including the last should have a trailing comma.

Also, the empty array can be represented as [], [ ], array() and array( ). I use the first, but the second is suddenly appealing...

I've tended to use [] for empty arrays, [ 'key' => 'value' ] with spaces for single element assignments, but I'm open to switching to [ ] for consistency.

Oh, and we need an opinion on whether or not the double-arrow => should be aligned, and if so, how!

I've tended not to align it. I'm skeptical that it actually lends to readability well enough to justify the effort it takes to implement it. I'll entertain arguments for other views, though. :)

Answer 5 · 2015-05-25T20:26:53.000Z

For issues that impact many or all repos (e.g. those affecting configuration files for testing common to all repos), we should probably specify that they should be filed against either this repo or some other repo dedicated to that purpose.

Answer 6 · 2015-08-26T18:57:09.000Z

I was configuring php-cs-fixer, since i have a few problems to follow others' standards, and got this configuration:

/*      // This must be used with 'header_comment', but then a config file is needed for each repository
$header = <<<EOF
Phergie (http://phergie.org)

@link http://github.com/phergie/phergie-irc-bot-react for the canonical source repository
@copyright Copyright (c) 2008-2015 Phergie Development Team (http://phergie.org)
@license http://phergie.org/license Simplified BSD License
@package Phergie\Irc\Bot\React
EOF;

Symfony\CS\Fixer\Contrib\HeaderCommentFixer::setHeader($header);
*/

return Symfony\CS\Config\Config::create()
    // All PSR-1 and PSR-2 fixers are included here.
    ->level(Symfony\CS\FixerInterface::PSR2_LEVEL)

    ->fixers([
        //  Accepted styling
        'short_array_syntax',               // Arrays should use the PHP 5.4 short-syntax.
        'multiline_array_trailing_comma',   // Multi-line arrays should have a trailing comma.
        'single_array_no_trailing_comma',   // Single-line arrays should not have trailing comma.
        'unalign_double_arrow',             // Unalign double arrow symbols.

        //  Suggested styling
        //'single_quote',                   // ? Convert double quotes to single quotes for simple strings.
        'standardize_not_equal',            // Replace all <> with !=.
        'unalign_equals',                   // Unalign equals symbols.
        'unused_use',                       // Unused use statements must be removed.
        'whitespacy_lines',                 // Remove trailing whitespace at the end of blank lines.

        //  Non-essential styling
        //'header_comment',                 // ? Set header comment from this file on php files
        'phpdoc_order',                     // Annotations in phpdocs should be ordered so that param annotations come first, then throws annotations, then return annotations.
    ])

    ->finder(
        Symfony\CS\Finder\DefaultFinder::create()
            ->in(__DIR__.'/src')
    );

This is not a final configuration, and there others thing to add if you like, i just got the minimal structure based on this discutions and code reading. Also, i added some some coding stardard sugestions to be formally accepted, most of them are already use as i could see, the exception is 'single_quote' that was caught in some script's runs.

Finally the 'non-essential' are things that could be used in .php_cs, but don't need to be explained in a Contribuition file. In case of 'header_comment' i don't recommend use it, because this way must be a configuration file for every phergie project, i just thought this could get your interest.

Naming style can't be handled by the script and (probably) use the following style:

classes, interfaces => CapitalizedCamelCase
methods, properties, variables => camelCase
event naming => Event names follow the convention namespace.subnamespace.event using a root namespace equal or similar to the plugin emitting the events and as many subnamespaces as are needed / reasonable.
plugins option naming => slugfied style my-custom-option

[NOTE] I don't think this configuration file must be in all projects, but maybe in scafolding this will help.

Answer 7 · 2015-08-26T19:04:14.000Z

@enebe-nb Awesome, thanks for putting this together!

@phergie/owners Any comments from the peanut gallery?

Answer 8 · 2015-08-26T19:17:42.000Z

This looks good to me. We should include the cs-fixer-file in the repo IMHO and even configuring an .editorconfig file to show this stuff in real time.

Answer 9 · 2015-08-26T19:29:22.000Z

Im in agreement with @svpernova09

Answer 10 · 2015-09-04T02:29:29.000Z

Weighing in here. My personal preference is PSR-2 and outside of that whatever is best for readability, where that can be subjective I would lean towards established conventions unless otherwise talked out of it.

Established:

indent = 4sp
methods, properties + variables = camelCase
K&R braces (including elseif/else)
binary operators with constants - the constant comes to the right of the binary operator (eg. $var === null)

What I would like see us agree on:

Array syntax is [] or [ 'key' => 'value' ]
As stated in PSR-2#6 Closures would appear function ($args) use ($vars) {

To be discussed:

Should there be a unified convention for naming custom events, or should plugin developers be free to name them at their discretion?

I agree with @elazar on "event names follow the convention namespace.subnamespace.event"

Indentation of chained method calls ($foo->setBar($bar)->setBaz($baz)->setQuux($quux);)

IMHO I almost alwys prefer to chain these after the first chained call.

Example:

$foo->setBar($bar)
    ->setBaz($baz)
    ->setQuux($quux);

Should all setters be chainable as a convention?

I have no opinion here.

Indentation of multi-line condition statements? Should the parentheses of multi-line condition statements be inline, or be proceeded/preceded by newlines?

IMHO the parentheses of multi-line condition statements should be in line. The whitespace by newlines I find can be distracting.

Should the operator be appended to the end of the previous line, or prepended to the front of the next line?

I really prefer the operating prepended to the front of the next line so you can read left -> right -> top -> bottom and easily see what's happening.

Example:

$sentence = $greeting
    . $comma
    . $space
    . $name
    . $period;

Answer 11 · 2015-09-04T03:01:42.000Z

Once we've nailed down the code style we should look into https://styleci.io/ for automatic checking for PRs and such.

Might be easier / cheaper (since Style-CI isn't free) to add something to check via travis

Answer 12 · 2015-09-04T05:26:42.000Z

As stated in PSR-2#6 Closures would appear function ($args) use ($vars) {

I think this is already accepted, since everyone agrees we should follow PSR-2.

Array syntax is [] or [ 'key' => 'value' ]

About the empty array, both elazar and renegate are using [] so i see no reason to change to [ ] now. Also cs-fixer converts array() to [], so its easier to use this style.

Once we've nailed down the code style we should look into https://styleci.io/ for automatic checking for PRs and such.

Might be easier / cheaper (since Style-CI isn't free) to add something to check via travis

The cs-fixer already converts the coding, the only problem is run it after merge a pull request (or before). And we can create customs filters for it (when a builtin filter is not avaliable), i just don't recommend use filters that can change the code behaviour. Pick your choices on styling and on a moment i have time i can edit the configuration.

Answer 13 · 2015-09-07T23:04:43.000Z

Updating our progress so far:

Established:

indent = 4sp
methods, properties + variables = camelCase
K&R braces (including elseif/else)
binary operators with constants - the constant comes to the right of the binary operator (eg. $var === null)
Array syntax is [] or [ 'key' => 'value' ]
As stated in PSR-2#6 Closures would appear function ($args) use ($vars) {
Event names should follow the convention namespace.subnamespace.event
The parentheses of multi-line condition statements should be in line.
Multi line operations: The operator should be prepended to the front of the next line.

To be discussed:

Indentation of chained method calls ($foo->setBar($bar)->setBaz($baz)->setQuux($quux);)

IMHO I almost alwys prefer to chain these after the first chained call.

Example:

$foo->setBar($bar)
    ->setBaz($baz)
    ->setQuux($quux);

If anyone would like to raise issue with anything we have established so far please feel free to do so.

Answer 14 · 2015-09-08T13:14:09.000Z

For reference, maybe have a look at this configuration here: https://github.com/refinery29/php-cs-fixer-config.

Answer 15 · 2015-10-01T21:22:02.000Z

Closing the discussion here, please review initial CONTRIBUTING.md contained here: #38