-
-
Notifications
You must be signed in to change notification settings - Fork 131
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add accumulate exercise #111
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the pull request! Ideally we only want the Accumulate exercise adding in this PR, not the Binary exercise. Could you remove those commits from this PR? Let me know if you want a hand with this and I'd be happy to help.
$this->assertEquals(['Hello', 'World!'], accumulate(['olleH', '!dlroW'], $accumulator)); | ||
} | ||
|
||
public function testAccumulateContants() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should that be testAccumulateConstants()
?
require_once 'accumulate.php'; | ||
|
||
// These required for last three tests | ||
require_once 'test_stub_static.php'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Requiring other files (other than the example) isn't going to pass the build as we use the Makefile to copy the example and the test to a new test dir. You could just add these classes inline to the bottom of the test file - that would then pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is exactly how it was done it the first place (mock classes at the bottom of the test). But again, like in #110, the code sniffer was complaining that every class should go in its own file (which makes total sense but not for tests) and killing travis builds.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh for goodness sake! Hmm, maybe we need to have a look at the code sniffer rules. Lets raise it as an issue
This reverts commit d62f7df.
I reverted the commits that came with the binary exercise and committed the proposed change in #113 in order to make sure that we're all green. Please let me know if anything else should be done before it's merged. Thanks. |
Hi @pmatseykanets this now looks great but I can't merge it because it will conflict with the other recent merges - |
@petemcfarlane I reconciled it with the upstream. Please merge. Thanks. |
Thanks! |
* Add binary exercise. * Change casing to camel case. * Add binary exercise. * Add accumulate exercise. * Fix CS. * Revert "Add binary exercise." This reverts commit d62f7df. * Move mock classes back into the test file. * Allow multiple classes per file. Solves exercism#112 * Remove remnants of binary exercise from config.json. Solves exercism#112 * Refactor tests.
No description provided.