julmud
commented
4 years ago It was more for me a test of enabling GitHub Actions on a project. It does just a simple lint pass (i.e., executes "php -l" on all php files), so it'll just prevent unparseable code from landing on master.
I'll see to add an .editorconfig that matches as best as possible the legacy codebase. As for doing more reformatting / linting, I'd say we should stay with option B for now.
(BTW, you seem to have a way bigger knowledge of the PHP ecosystem than I do... I'd never heard of phpCodeSniffer and phpmd before, but I hadn't done any PHP code writing for quite a few years now ;-) )
On Mon, 6 Jan 2020 at 09:54, Gavin-John Noonan notifications@github.com wrote:
I will test/review this today, but my initial first impressions are that whilst I agree that adding an action to lint is always a good idea, I tried running my usual linting tools (phpCodeSniffer, phpmd, others) before I pushed my code and there are so many style violations the added code in the MR would have been lost in the noise of the diff.
In a legacy codebase such as this there’s a couple of options;
A) A style (PSR-2 generally) is agreed or set down by the project owner, and then there is a big MR bringing all the code in line to this. The style is then set in stone, documented, and you have linting pre commit, merge, etc.
B) agree that new code/ features should meet the standard, but if you’re editing the old code/ files then just keep it consistent with the code around it, as bad as it may be.
In addition, if we are linting we should also add a .editorconfig in the route that defines the style for tabs/spaces, line length, and LF/CLRF
Sent with GitHawk http://githawk.com
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/julmud/phpDVDProfiler/pull/3?email_source=notifications&email_token=ABDBBUMFM6Z5Q3I3IUNLSADQ4LWSTA5CNFSM4KDAIQH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEIE2ILY#issuecomment-571057199, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABDBBUI4XOR2LZEJUWQ6MFLQ4LWSTANCNFSM4KDAIQHQ .
gjnoonan
I will test/review this today, but my initial first impressions are that whilst I agree that adding an action to lint is always a good idea, I tried running my usual linting tools (phpCodeSniffer, phpmd, others) before I pushed my code and there are so many style violations the added code in the MR would have been lost in the noise of the diff.
In a legacy codebase such as this there’s a couple of options;
A) A style (PSR-2 generally) is agreed or set down by the project maintainers, and then there is a big MR bringing all the code in line to this. The style is then set in stone, documented, and you have linting pre commit, merge, etc.
B) agree that new code/ features should meet the standard, but if you’re editing the old code/ files then just keep it consistent with the code around it, as bad as it may be.
In addition, if we are linting we should also add a .editorconfig in the route that defines the style for tabs/spaces, line length, and LF/CLRF