Code quality: Makefile to run static code checkers

[virtualtam]

Here's a Makefile to run the static analysis tools mentioned in #95.

• Several configurations are available for each tool, I've tried to keep them as relevant as possible.
• Tools are managed with Composer, which is pretty straightforward to use.

There's also a Travis config draft, untested (Travis is currently under maintenance).

Edit: Travis is back! Here's what a build looks like: https://travis-ci.org/virtualtam/Shaarli/builds/52341101

[nodiscc]

The travis log/make output at https://travis-ci.org/virtualtam/Shaarli/builds/52341101 doesn't provide any useful information for debugging, refactoring, cleaning up code... How can we use it to improve Shaarli?

[virtualtam]

It provides stats, in a concise way, about how compliant the code is to clean coding standards.
More detailed output is available, for each tool, with other Makefile targets, and should be kept for local use.

I've been using that kind of utility as a commit hook for some time, and it's great to proof-check your commit/branch status before actually pushing anything anywhere (e.g. local/small projects without code-review services).

Not having details in commit-triggered builds is useful for project managers, to track code evolution trends.

The main goal behind having a Makefile is to easily run unit tests, which is planned for #71. You may then consider having static checkers' concise output as a bonus for your build log 😃

Please note that I'm not assuming everything here is relevant; this PR essentially provides a fashionable way to install and run some popular PHP dev tools.

[nodiscc]

Ok, while I see (and approve) the goal of this PR (better code quality), I'm not ok with merging this in the master branch, which is our current release branch.

Dev tools have nothing to do in our released code. The Makefile is definitely useful to run checks locally and work on code quality. A dev-tools branch + docs on how to install composer and update requirements + docs on how to run checks would be a better place. Or a dedicated wiki page.

[virtualtam]

I'm a bit confused...

What would be the point in hiding test code in a development branch? Or even worse, a Wiki page?

Providing test code and utilities along the actual product's codebase is, besides being a standard for all modern projects, a huge asset to improve the quality and quantity of code contributions:

• the presence of tests usually means that a project is being well-cared of;
• designing, writing and running tests should be as straightforward as possible;
• any tool necessary for development and / or helping maintaining a clean codebase should be easily installable and usable by contributors.

Tests are part of -and often more important than- the code: they live with it, show how it (is expected to) work, and should not be kept apart.

There may be a confusion here, between:

• a release branch, which is the state the software is when you flag it as being ready for release,
• the actual released product, which is what the user gets.

Here is an awesome example of a successful Git branching model:

• master: stable branch;
• devel: development mainline (next release);
• v1.x: release branches;
• feat/topic/feature: feature branches;
• fix/topic: hotfixes.

Given the small size of Shaarli, we could stick to stable, development and release branches.

As I understand it, the issue is having "non-release" code provided to users; why not work on a release script / process then, that would:

• trim dev resources, test code and utilities;
• minify stylesheets, only keep minified Javascript libs;
• build an all-in-one archive, ready for the user to grab and deploy.

[nodiscc]

why not work on a release script / process then, that would

This is something that could be done in the long run. However the current download strategy for users is to either git clone the repo, or grab and extract the zipball that github provides. I'd prefer not changing it, regarding the current state of Shaarli where there are already issues that need to be dealt with before thinking about adding external tools like travis, or complicating the release process.

While the travis integration is nice and automatically gives us a summary of what could be improved on each pull request, it does not help with the current state of the code at all, which needs to be addressed before we can think of best practices for further contributions.

How I'd handle that: first merge the Makefile since it is helpful for cleaning up current code. Provide comments in it on how to install the required tools. Let the composerfile and travis integration wait for later.

[nodiscc]

@pikzen seemed to disagree with me in a private chat:

(17:10:02) Monsieur //pikzen: jpeux comprendre
(17:10:16) Monsieur //pikzen: c'est pas parce que t'as dla merde sur la chaussure que tu les cire pas

I say wash first.

[pikzen]

huehuehuehue
I expanded a bit later though.

For those who do not speak french, it went that way:

Monsieur //pikzen: I can understand
Monsieur //pikzen: Having shit under your shoe does not prevent you from polishing it
Monsieur //pikzen: analogy.png
...
Monsieur //pikzen: I mean, you still have shit under your shoe
Monsieur //pikzen: but at least it's polished

I definitely agree with providing tools to maintain a high code quality in Shaarli (read: not what there is currently). PHPMD is great (835 problems on the latest master woohoooo), so is phpcs, etc.

What @nodiscc probably didn't understand is that I rather agree with him (which also means that my analogies are terrible): providing the Makefile is great.
I'm slightly less convinced by Travis because you're only warned once the code is pushed, which makes little sense to me, the push shouldn't have any warning. I can understand that some people might not want to have to install phpmd, phpcs et. al. to contribute to the code. I don't care personally, my dev machine probably has half of debian's packages installed.

Providing an install script is great: we can store whatever the hell we want in master and not have it mirrored when the users install Shaarli.

(As an aside, phpmd reports $GLOBALS usage as a problem, so either ignore it or merge #108 in to fix 300 in one go )

[nicolasdanelon]

Shaarli runs with PHP 5.1
Travis-cli PHP 5.4 right ?
And Symfony/Yaml PHP 5.5.9
So... is this good for the project ?

[e2jk]

I do think that having this included in the master repository is the right thing to do. It allows anyone getting the source to reproduce it (which is a core part of the FLOSS philosophy, we're not just dumping a tar-/zipball over the fence every now and then), can be used by Linux distributions to run tests, etc. when building their packages, and provides a way to automatically run these tasks each time a new commit is pushed to GitHub.

As I have explained elsewhere, the original goal of having everything in one file is long gone. And in the worst case the user has a Makefile on their webserver. Not a big deal, they already have these files that are not strictly needed (in the technical sense) to run the software:

• README.md
• .gitignore
• COPYING
• inc/LICENSE

[nodiscc]

Fine, let's merge it if you think it helps. At least I agree the Makefile helps :)

[virtualtam]

Thanks to all for you insights ;-)

you're only warned once the code is pushed

That's the cool thing, Travis runs builds on any branch that has a .travis.yml, and for every pull request (I haven't tested it yet, feel free to submit dummy PRs on my repos to see what happens ^^)

first merge the Makefile

Agreed. Travis integration was a bonus / demo here, and can be kept for later.

X runs with PHP 5.Y

Adding expected PHP revisions to Travis is quite easy, I think we can safely focus on PHP >= 5.4, at least as a starter (CI evolves with the software, and is no time machine)

Provide comments in it on how to install the required tools.

I'd rather provide an actual Composer config file, than a Makefile with lots of superfluous setup comments / instructions.
The aim is to give devs helpful tools, not to hassle them with installing dependencies and setup a test environment.

my dev machine probably has half of debian's packages installed

4-year-old dev ArchLinux here, quite safe to assume there's a bunch of unused and obsolete stuff lurking in each and every corner

If needed, we could meet by IM to discuss CI PRs (either on Gitter, IRC, XMPP or whatever service is convenient) 😃

[nodiscc]

@virtualtam @e2jk So what do we keep in this Pull Request before merging?

[virtualtam]

I'd:

• keep the Makefile, gitignore and composer files,
• improve usage notes in the Makefile, to better describe which tool does what (though it's quite explicit already),
• start a wiki page, e.g. How to contribute to Shaarli, where we'd define some guidelines to follow for PR submission,
• remove the Travis config file, and keep it for an other PR (I've seen it work, which is a pleasant start)

[nodiscc]

@virtualtam Ok, please make the required changes and rebase and we can merge. If you want to document the contribution process (in the wiki) it's also welcome, either in a separate page or in a new chapter.

[virtualtam]

PR updated ;-)

[nodiscc]

Thanks!