-
-
Notifications
You must be signed in to change notification settings - Fork 138
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
Update Accumulate to use namespaces #287
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.
Great PR, thank you! Just some small tweaks / clarifications as mentioned.
@@ -1,11 +1,13 @@ | |||
<?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.
DId you leave the example file in the execise's root on purpose? Personally I would place it in src
, because there I would look for it and it makes adjusting Makefile
a little easier.
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.
On second thought:
Not sure, if the exercise root folder would still be the better place. I have to sleep over it...
Is there an exercise, where the example solution requires multiple files (more than one class)? That would require some thought on how we can solve this. In that case an example directory might be the better solution.
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.
Isn't that more part of how exercism works regardless of the language?
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.
Not sure, I am mainly on the PHP track for now. We leave it here for now, that's fine.
Split test classes to their own files
11b3ae0
to
36b9620
Compare
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.
LGTM
@petemcfarlane We've made a couple of PRs here updating to using src and test directories as well as naming the files in a way that matches PHP PSR-4 standards. However, I didn't know or notice there's a makefile that specifies the names of the example and test file. I'm not sure how or if we should proceed with these. The Accumulate (this one) and Acronym PRs have been updated this way, but they won't work with the existing makefile and I'm not sure of a great way forward short of updating everything all at once. Any thoughts or ideas here? |
If I may add some additional information for clarification: The goal of this PR is to provide a POC for #283. Other exercises would need an update as well. Preferably in one PR. @petemcfarlane The primary question we have, is, what your thoughts are on this and whether you can support this effort? Or have doubts/concerns? This PR currently fails test (Travis-CI was somehow not triggered). The issue is that the changed directory structure is not compatible with the Makefile configuration. We have two thoughts:
|
If there's a way to do all this work where it doesn't require it to be in on PR, that would be preferred. One PR doing all this would be massive and impossible to accurately review. |
Once we agree on a structure, it should be pretty straight forward. But again, I have not completed all exercises yet and don't know for sure if there is an exercise that raises the complexity in changing to namespaces. It might be best to develop against a separate branch and then merge back to master as soon as everything is fine. |
Closing this. I think much of what this was trying to accomplish has been handled elsewhere and more recently. |
Split test classes to their own files
@arueckauer - Thoughts on these changes?