Add some standards and best practices to CONTRIBUTING

[wpillar]

Proposing we add the additional standards and best practices to our CONTRIBUTING file. The reason behind them is so that we are all coding consistently which in turn makes it easier to develop the codebase as a whole.

Some of them are also conventions of other PHP projects so it brings more inline with the community as well, which I always think is a good thing :)

Let me know what you think, happy to explain the reasoning behind each of them individually. Would be great to see this merged!

[euantorano]

Looks good to me.

On 23 Mar 2015, at 21:33, Will Pillar notifications@github.com wrote:

Proposing we add the additional standards and best practices to our CONTRIBUTING file. The reason behind them is so that we are all coding consistently which in turn makes it easier to develop the codebase as a whole.

Some of them are also conventions of other PHP projects so it brings more inline with the community as well, which I always think is a good thing :)

Let me know what you think, happy to explain the reasoning behind each of them individually. Would be great to see this merged!

You can view, comment on, or merge this pull request online at:

https://github.com/mybb/mybb2/pull/31

Commit Summary

Add some standards and best practices to CONTRIBUTING File Changes

M CONTRIBUTING.md (42) Patch Links:

https://github.com/mybb/mybb2/pull/31.patch https://github.com/mybb/mybb2/pull/31.diff — Reply to this email directly or view it on GitHub.

[JN-Jones]

TBH I still think we shouldn't force to use getters and setters. Laravel saves attributes in an protected array and uses __get/__set already as getter/setter. And it still allows casting or the use of mutators if you need to do something special. Having an extra function which only calls the magic getter/setter seems useless to me. (Note: speaking of models here, of course other classes without the magic getter/setter should've them).

Also I think we should mention that Repositories and Factories should have a suffix too. And we need to note how to handle repositorie interfaces: XyzRepositoryInterface or XyzInterfaceRepository? (First should be used but it should be mentioned)

Methods with a return value and/or arguments MUST have a document block. Probably better "SHOULD have"? There may be cases where it's pretty obvious (eg hasAttributeXy() will pretty sure return a boolean).

Also someone needs to apply these changes to the current files ;)

[euantorano]

Can we have some more comments on the use of getters and setters? Having them (as well as the existing magic __get and __set methods provided by Eloquent) on models would mean two calls on the call stack to retrieve a parameter.

I'm not sure what to think. I'm currently meaning towards getters and setters for everything apart from eloquent, with mutators used whenever custom logic is needed to get/set a model parameter.

[euantorano]

Also I think we should mention that Repositories and Factories should have a suffix too. And we need to note how to handle repositorie interfaces: XyzRepositoryInterface or XyzInterfaceRepository? (First should be used but it should be mentioned)

Yes. The format should be as follows: XyzRepositoryInterface. Factories should be named XyzFactory, and repositories should be XyzRepository. These rules should be added to the standards.

[wpillar]

At the weekend I'm going to make sure that accessing Eloquent properties directly is just as unit testable as a getter that can be mocked. If it's just as easy to test then I'm probably fine with getters and setters except for eloquent models.

[wpillar]

Ok, so here is how you mock an Eloquent property:

$setting = Mockery::mock('\MyBB\Settings\Models\Setting');
$setting->shouldReceive('getAttribute')
    ->with('value')
    ->andReturn('bar');

And here's how do you it with a getter:

$setting = Mockery::mock('\MyBB\Settings\Models\Setting');
$setting->shouldReceive('getValue')
    ->andReturn('bar');

I still prefer the getter but I don't mind accessing the property given that it's not that difficult to mock. Have updated the PR to reflect this.

Please can we have the final round of comments on the amended PR and move to merge? :)

[wpillar]

@JN-Jones happy to take on the task of changing existing files to meet the requirements I have added in this PR.

[JN-Jones]

Can you add the factory/repository suffix rules?

Also what's with the doc block I mentioned above? IMHO functions like hasAttributeXy() don't need to have a doc block as it's pretty obvious what they do and return. Or is there any other reason to include them?

[wpillar]

@JN-Jones ah yes, sorry forgot you mentioned those.

On the point of the doc block, my IDE does not know that has*() methods should return bool. With a doc block, it does, and is then able to more easily tell me where I've cocked up. It's also good for documentation, should we ever want to auto-generate some API documentation alá http://laravel.com/api/5.0/.

If you're using a good IDE or editor then adding the doc blocks shouldn't be taxing to add. Should also stress that the doc block requirement is only for the @return and @param tags, it does not mandate that the doc block have anything other than those tags unless appropriate. So you shouldn't end up with lots of has*() methods with descriptions like Does this foo have a bar? because they are a bit useless I agree.

[wpillar]

@JN-Jones updated the PR with the other suffix conventions.

[JN-Jones]

What's with a factory repository or even an interface for one? :P I think we can leave them for now as I don't think we'll ever have one.

For the doc blocks: ok, methods should have one. What's with properties/classes? IIRC @euantorano added a simple block for each property and none for the classes while you added one for the class.

[wpillar]

We don't need a convention for a factory repository or an interface for one.

ForumFactoryRepository - A repository of forum factories. ForumRepositoryFactory - A factory that builds forum repositories.

They have different meanings depending on the order. Those two classes could happily co-exist side by side and as for interfaces of those, that's covered by the interface priority suffix convention.

[wpillar]

As for doc blocks for properties, the PR mandates that object properties have a doc block specifying a @var tag declaring the type of the property. Which I stand by.

If you're referencing this:

/**
 * @property string id
 * @property string profile_field_group_id
 * @property string validation_rules
 * @property string name
 */
class ProfileField extends Model implements HasPresenter
{
}

That is because they are magic properties and not proper object properties. So there is no property I can assign an individual doc block for, so I declare them here at the top of the class so my IDE still recognises them. Do you think we should add that to the standards also?

My position is that all proper, declared object properties should have individual doc blocks but magic properties should be declared in the class doc block as above.

[JN-Jones]

Ah ok. I'd like to see those magic property blocks however there may be an issue with models as they can have quite a lot: the attributes, relations, scopes, mutators for nonexistent attributes. Let's see what @euantorano thinks about them.

[wpillar]

Yeh there will be models that have a lot of them, but I personally don't think that's an issue. Again, if you've got a good IDE / editor, trivial to add them as you go.

[euantorano]

Looks good to me. We should definitely be annotating properties on models (or anywhere else using magic methods for properties). It's a major annoyance otherwise.

[JN-Jones]

Ok, so this can be merged after the properties are added imho

[euantorano]

Yep. Would be nice to get it merged :)

[wpillar]

Updated. All good to merge? @euantorano @JN-Jones ?

[euantorano]

Consider it done :+1:

[wpillar]

Nice, thanks. I'll add a task to my list to update existing violations of what I've added here.

[euantorano]

Yeah, we'll need to go through everything already done and fix it up. I started changing a few things earlier, but it'll be a big job.