Conform to PSR-2, add Composer/PHPUnit configs #11

jrdnull · 2014-07-07T12:10:30Z

I've removed the two new exercises and squashed this into one commit, this PR reformat to PSR-2 standards.

The classes are now autoloaded with the phpunit.xml and composer.json provided. (This is done using Composer's autoloader)

The make file has been updated so only the tests are rewritten to the tmp dir and they are modified on the way to not skip tests and to require in the classes needed so it doesn't require an autoloader. (I'm sure this can be done in a nicer way as this is a bit hacky)

I am unsure how the CLI tool provides files, is it possible that when fetching PHP exercises that the composer.json & phpunit.xml are provided as well as perhaps a language specific README? That way when a user came to do some PHP exercises they would have a standard quick and easy way to get PHPUnit and the tests running. e.g:

% curl -sS https://getcomposer.org/installer | php
% php composer.phar install
% php vendor/bin/phpunit [Exercise]

Let me know any problems/improvements and I'll work on it.

jrdnull · 2014-07-07T12:36:57Z

I'll hold off creating pull requests for the two new exercises until this is sorted as they are based off this branch. The exercises I have added the hamming exercise and UncleBob's bowling game kata to avoid anyone duplicating the effort.

wilmoore · 2014-07-08T06:08:52Z

Makefile

@@ -3,26 +3,29 @@ IGNOREDIRS := ".git|vendor|bin"
 ASSIGNMENTS = $(shell find . -maxdepth 1 -mindepth 1 -type d -not -path '*/\.*' | tr -d './' | sort | grep -Ev $(IGNOREDIRS))

 # output directories
+DIR := $(shell pwd)


You may be able to use the builtin $(CURDIR) environment variable.
https://www.gnu.org/software/make/manual/html_node/Recursion.html

jrdnull · 2014-07-08T07:32:01Z

@wilmoore I've made that change, also I've changed it back to fetch phpunit and test with that passing the --no-configuration flag so it doesn't expect the autoloader to be there.

I've also added a SETUP.md, assuming that composer.json and phpunit.xml will be available to the user in the {exercism download dir}/php dir.

kytrinyx · 2014-07-08T16:47:04Z

This looks good to me. I'm going to give the final word to @wilmoore since he's written PHP more recently than I have.

jrdnull · 2014-07-08T20:08:09Z

Sounds good, I've added the leap problem to another branch for if/when this is merged.

@kytrinyx is there the functionality in place at the moment to provide the composer.json & phpunit.xml files in the base directory of the language exercise folder?

wilmoore · 2014-07-08T23:18:59Z

Makefile

+	@for class in $(shell ls $(ASSIGNMENT)/*.php | grep -v Test); do \
+		echo "<?php require_once '$(CURDIR)/$$class'; ?>" >> $(OUTDIR)/$(TSTFILE); \
+	done
+	@sed -e "/$this->markTestSkipped/d" -e "/namespace/d" $(ASSIGNMENT)/*Test.$(FILEEXT) >> $(OUTDIR)/$(TSTFILE)


I'm not sure I love this methodology. I think we could do better by utilizing phpunit's @group in tandem with --group and/or --group-exclude cli options.

So instead of marking the tests as skipped give them a group annotation such as @group skipped and ask the user to remove the annotation rather than the $this->markTestSkipped line?

So the user would run tests with (Edit: we can just specify it in the phpunit.xml so they don't have to pass it)

$ php vendor/bin/phpunit Exercise

The @group comment annotation would be easier/quicker to remove than removing a line inside the test method. I'll make another commit to switch to this method of excluding the other tests.

...give them a group annotation such as @group skipped and ask the user to remove the annotation rather than the $this->markTestSkipped line?

Yes, and also keep in mind that:

If phpunit.xml or phpunit.xml.dist (in that order) exist in the current working directory and --configuration is not used, the configuration will be automatically read from that file.

Which means we could potentially allow the user to not worry about it and utilize a phpunit.xml that does what we want.

kytrinyx · 2014-07-08T23:20:52Z

There's no support for that at the moment. It should be trivial to add it to the API side of things, and we'll need to add functionality to the CLI. That's not a big deal, it just means that we'll have to specify to people doing the PHP track that they'll need the most recent version of the CLI when they get started.

kytrinyx · 2014-07-08T23:55:23Z

I've added a couple of issues to add support for this:

https://github.com/exercism/x-api/issues/27
exercism/cli#88

wilmoore · 2014-07-09T00:26:19Z

Trinary/Example.php

+
+    public function __construct($decimal = '')
+    {
+        $array = array_reverse(str_split((string)$decimal));


This isn't a deal breaker, but my feeling is that there are a lot of nested parens here. Perhaps breaking it out would be nice.

Also, I believe that psr-1 doesn't like (string)$decimal since there is no space between the paren and $decimal.

I haven't really looked too much at these exercises, I just ran autoformat against them. What about if the cast was removed as it shouldn't be needed anyway?

$array = array_reverse(str_split($decimal));

I would even combine that to:

public function __construct($decimal = '') { $this->digits = array_map('intval', array_reverse(str_split($decimal))); }

PSR-1/2 doesn't seem to care about the space between the cast, I couldn't find an example in the standards and CodeSniffer and PHPStorm rulesets do not point it out.

I just ran autoformat against them

I generally feel like auto-formatting is great if the recipe is sharable. Perhaps we can use: https://github.com/fabpot/PHP-CS-Fixer

I like this because there are also dedicated plugins for multiple IDE/editors available or the CLI can be run directly.

Let's set this up since we've already moved in the direction of utilizing auto-formatting on the code.

What about if the cast was removed as it shouldn't be needed anyway?

Yes, let's drop it given it isn't really necessary.

I would even combine that to

👍

PSR-1/2 doesn't seem to care about the space between the cast...

I guess it's a moot point if we remove it though 😄

I've added PHP_CodeSniffer to the build so any violations should fail. I'm not sure how we could have PHP-CS-Fixer run, perhaps add a link to it in the README to make contributors aware of it?

I'm not sure how we could have PHP-CS-Fixer run, perhaps add a link to it in the README to make contributors aware of it?

Yes, that should be good enough; though, seems like this may be better placed in the SETUP.md file.

better placed in the SETUP.md file

IIRC didn't @kytrinyx say we shouldn't influence the style used in users solutions?

IIRC didn't @kytrinyx say we shouldn't influence the style used in users solutions?

That may be true...if so, then so-be-it.

@kytrinyx -- would you prefer user style be fairly free-form?

@wilmoore this was the comment I was referring to, is there anything else needs to be done with this PR?

jrdnull · 2014-07-09T08:38:19Z

@wilmoore I think this should address all of the issues, I have created an issue to discuss the 5.6 features here: #12

Let me know if there is anything else, cheers.

wilmoore · 2014-07-11T16:17:47Z

Let me know if there is anything else, cheers

LGTM 👍

wilmoore · 2014-07-11T16:18:04Z

@jrdnull - just squash and I'll merge.

jrdnull · 2014-07-11T17:47:38Z

@wilmoore squashed 👍

@group

Update Makefile, add SETUP.md Add PHP CodeSniffer Use @group annotation to skip tests

Conform to PSR-2, add Composer/PHPUnit configs

wilmoore · 2014-07-11T19:34:40Z

Thanks a lot @jrdnull -- great work 👍

wilmoore · 2014-07-11T19:51:36Z

FYI, @jrdnull - I notice that the stuff in SETUP.md doesn't quite work for me. For example:

% php vendor/bin/phpunit Trinary

When I run that, I get:

PHP Fatal error: Class 'Exercism\Trinary\Trinary' not found...

jrdnull · 2014-07-11T19:56:27Z

Have you created Trinary.php? You'll have to use the make test to run the tests against the Example.php's. I guess the instructions could be clearer.

wilmoore · 2014-07-12T23:01:45Z

I guess the instructions could be clearer.

Yeah, there is that, plus I am not sure I love the re-writing of the source code. I'll try to work on this when I have some spare cycles. If you get to it first, that's even better.

wilmoore · 2014-07-12T23:04:23Z

I am not sure I love the re-writing of the source code

Additionally, I don't love having multiple ways to run the tests. I'd prefer to see either of:

make test-assignment ASSIGNMENT=AssignmentName
phpunit AssignmentName

But not both.

jrdnull · 2014-07-13T09:21:35Z

I am not sure I love the re-writing of the source code

The only way to avoid this is to remove autoloading and rename the file like it used to, or have an additional autoloader for the examples. Although this is a now a moot point due to #17.

Additionally, I don't love having multiple ways to run the tests.

We're still going to need two ways of running the tests to allow the CI to test against the example implementations.

wilmoore · 2014-07-13T09:45:33Z

We're still going to need two ways of running the tests to allow the CI to test against the example implementations

I should have been more specific. I could have summed it up by saying we need to get rid of the extra code re-writing which we do in #18.

Conform to PSR-2, add Composer/PHPUnit configs

wilmoore reviewed Jul 8, 2014
View reviewed changes

kytrinyx mentioned this pull request Jul 8, 2014

Add support for fetching files to root track dir exercism/cli#88

Closed

wilmoore reviewed Jul 9, 2014
View reviewed changes

Conform to PSR-2, add Composer/PHPUnit configs

3f748a2

Update Makefile, add SETUP.md Add PHP CodeSniffer Use @group annotation to skip tests

wilmoore added a commit that referenced this pull request Jul 11, 2014

Merge pull request #11 from jrdnull/psr2

33d23de

Conform to PSR-2, add Composer/PHPUnit configs

wilmoore merged commit 33d23de into exercism:master Jul 11, 2014

wilmoore added a commit that referenced this pull request Aug 31, 2014

Merge pull request #11 from jrdnull/psr2

6d11bf5

Conform to PSR-2, add Composer/PHPUnit configs

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Conform to PSR-2, add Composer/PHPUnit configs #11

Conform to PSR-2, add Composer/PHPUnit configs #11

jrdnull commented Jul 7, 2014

jrdnull commented Jul 7, 2014

wilmoore Jul 8, 2014

jrdnull commented Jul 8, 2014

kytrinyx commented Jul 8, 2014

jrdnull commented Jul 8, 2014

wilmoore Jul 8, 2014

jrdnull Jul 9, 2014

wilmoore Jul 9, 2014

kytrinyx commented Jul 8, 2014

kytrinyx commented Jul 8, 2014

wilmoore Jul 9, 2014

jrdnull Jul 9, 2014

wilmoore Jul 9, 2014

jrdnull Jul 9, 2014

wilmoore Jul 9, 2014

jrdnull Jul 9, 2014

wilmoore Jul 10, 2014

jrdnull Jul 11, 2014

jrdnull commented Jul 9, 2014

wilmoore commented Jul 11, 2014

wilmoore commented Jul 11, 2014

jrdnull commented Jul 11, 2014

wilmoore commented Jul 11, 2014

wilmoore commented Jul 11, 2014

jrdnull commented Jul 11, 2014

wilmoore commented Jul 12, 2014

wilmoore commented Jul 12, 2014

jrdnull commented Jul 13, 2014

wilmoore commented Jul 13, 2014

Conform to PSR-2, add Composer/PHPUnit configs #11

Conform to PSR-2, add Composer/PHPUnit configs #11

Conversation

jrdnull commented Jul 7, 2014

jrdnull commented Jul 7, 2014

Choose a reason for hiding this comment

jrdnull commented Jul 8, 2014

kytrinyx commented Jul 8, 2014

jrdnull commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kytrinyx commented Jul 8, 2014

kytrinyx commented Jul 8, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrdnull commented Jul 9, 2014

wilmoore commented Jul 11, 2014

wilmoore commented Jul 11, 2014

jrdnull commented Jul 11, 2014

wilmoore commented Jul 11, 2014

wilmoore commented Jul 11, 2014

jrdnull commented Jul 11, 2014

wilmoore commented Jul 12, 2014

wilmoore commented Jul 12, 2014

jrdnull commented Jul 13, 2014

wilmoore commented Jul 13, 2014