{"action":"created","issue":{"url":"https://api.github.com/repos/ddeboer/data-import/issues/201","labels_url":"https://api.github.com/repos/ddeboer/data-import/issues/201/labels{/name}","comments_url":"https://api.github.com/repos/ddeboer/data-import/issues/201/comments","events_url":"https://api.github.com/repos/ddeboer/data-import/issues/201/events","html_url":"https://github.com/ddeboer/data-import/pull/201","id":74717244,"number":201,"title":"Small improvements","user":{"login":"sagikazarmark","id":1226384,"avatar_url":"https://avatars.githubusercontent.com/u/1226384?v=3","gravatar_id":"","url":"https://api.github.com/users/sagikazarmark","html_url":"https://github.com/sagikazarmark","followers_url":"https://api.github.com/users/sagikazarmark/followers","following_url":"https://api.github.com/users/sagikazarmark/following{/other_user}","gists_url":"https://api.github.com/users/sagikazarmark/gists{/gist_id}","starred_url":"https://api.github.com/users/sagikazarmark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sagikazarmark/subscriptions","organizations_url":"https://api.github.com/users/sagikazarmark/orgs","repos_url":"https://api.github.com/users/sagikazarmark/repos","events_url":"https://api.github.com/users/sagikazarmark/events{/privacy}","received_events_url":"https://api.github.com/users/sagikazarmark/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":2,"created_at":"2015-05-09T17:21:32Z","updated_at":"2015-05-14T15:58:50Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/ddeboer/data-import/pulls/201","html_url":"https://github.com/ddeboer/data-import/pull/201","diff_url":"https://github.com/ddeboer/data-import/pull/201.diff","patch_url":"https://github.com/ddeboer/data-import/pull/201.patch"},"body":"This PR actually started to be a small fix in a few docblocks and ended up...... well, you can see.\r\n\r\nThese are just some basic improvents including:\r\n\r\n- removing comments like \"Constructor\"\r\n- removing method comments like: Get/Set \"the same name as the variable itself\"\r\n- removing trailing spaces\r\n- adding empty line to end of file\r\n- adding inheritdoc tag where possible (Based on examination of the code `{@inheritdoc}` is used more generally than `{@inheritDoc}` so I updated everything to the first version.)\r\n- fixing some PSR coding standard things\r\n- using sprintf where possible\r\n- using [] instead of array (PHP 5.4 array notation)\r\n\r\nLong list, but I have some further things on my list, which needs your feedback (@ddeboer, @Baachi)\r\n\r\n\r\n## Private or protected properties\r\n\r\nThere is a big inconsistency in the package related to variable visibility. Some classes use private, some use protected (while there is no indication why that specific visibility should be used). Generally I use protected (to allow extension) while I usually use decoration for example instead of inheritance. This is actually a really trivial one, but it seemed to worth mentioning it.\r\n\r\n\r\n## Validator or ValidatorInterface\r\n\r\nThe ValidatorFilter accepts a ValidatorInterface whereas the ValidatorStep only accepts a Validator instance. Any particular reason for that?\r\n\r\n\r\n## Introduce ReaderFactory interface\r\n\r\nIt seems natural to have an interface for that type of class as well\r\n\r\n\r\n## Add .editorconfig\r\n\r\nI am a big fan of [editorconfig](http://editorconfig.org/) which is supported by all major IDEs. How about adding one?\r\n\r\n\r\n## Use `integer` type hint instead of `int`\r\n\r\nThis is something that I argued about a lot with a few \"PSR, PSR, PSR\" (aka. do everything exactly the same way, any different logic is unacceptable) guy. I think we are writting software, thus the documentation should reflect that. `int` is a PHP keyword, the type name is `integer`. It is really unfortunate that in PHP integer is just s shortcut, the type is represented by the word int. Still, I suggest changing all occurences of `int` to `integer` (IIRC some IDEs use integer by default).\r\n\r\n\r\nHere come some BC breaking suggestions. Since the repo is going to be separated into a new organization (and keep in mind the steps PR is already a BC breaking one) I don't this is a problem.\r\n\r\n\r\n## Move AbstractWriter logic into a trait\r\n\r\nInheritance does not make sense here IMO.\r\n\r\n\r\n## Remove fluent interface where possible\r\n\r\nNot sure if you are going to like this one. I think that fluent interfaces make sense in a builder like environment. See [this](http://ocramius.github.io/blog/fluent-interfaces-are-evil/) post by @Ocramius.\r\n\r\n\r\n## Remove `Interface` suffix\r\n\r\nThis is another common pattern in the PHP community which I really see no point and dislike it. I know it is an internal standard for PSR guys, but... See [this](http://verraes.net/2013/09/sensible-interfaces/) post by @mathiasverraes. It contains everything I could say against it.\r\n\r\n\r\n## Use array typehint in StepInterface\r\n\r\nI am not sure about this one, but shouldn't the `process` method accept only arrays?"},"comment":{"url":"https://api.github.com/repos/ddeboer/data-import/issues/comments/102083387","html_url":"https://github.com/ddeboer/data-import/pull/201#issuecomment-102083387","issue_url":"https://api.github.com/repos/ddeboer/data-import/issues/201","id":102083387,"user":{"login":"sagikazarmark","id":1226384,"avatar_url":"https://avatars.githubusercontent.com/u/1226384?v=3","gravatar_id":"","url":"https://api.github.com/users/sagikazarmark","html_url":"https://github.com/sagikazarmark","followers_url":"https://api.github.com/users/sagikazarmark/followers","following_url":"https://api.github.com/users/sagikazarmark/following{/other_user}","gists_url":"https://api.github.com/users/sagikazarmark/gists{/gist_id}","starred_url":"https://api.github.com/users/sagikazarmark/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sagikazarmark/subscriptions","organizations_url":"https://api.github.com/users/sagikazarmark/orgs","repos_url":"https://api.github.com/users/sagikazarmark/repos","events_url":"https://api.github.com/users/sagikazarmark/events{/privacy}","received_events_url":"https://api.github.com/users/sagikazarmark/received_events","type":"User","site_admin":false},"created_at":"2015-05-14T15:58:50Z","updated_at":"2015-05-14T15:58:50Z","body":"@ddeboer @Baachi ping :)"}}