Skip to content
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

Closed
wants to merge 1 commit into from

Conversation

dstockto
Copy link
Contributor

Split test classes to their own files

@arueckauer - Thoughts on these changes?

Copy link
Contributor

@arueckauer arueckauer left a 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.

exercises/accumulate/test/accumulate_test.php Outdated Show resolved Hide resolved
@@ -1,11 +1,13 @@
<?php
Copy link
Contributor

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.

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

exercises/accumulate/test/accumulate_test.php Outdated Show resolved Hide resolved
Split test classes to their own files
Copy link
Contributor

@arueckauer arueckauer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dstockto
Copy link
Contributor Author

@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?

@arueckauer
Copy link
Contributor

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:

  1. Update Makefile (together with the changes for all exercises) to use the new directory structure.
  2. Introduce an example directory, which gets copied to src to run automated testing. This would be required, if multiple example solution files are required by an exercise.

@dstockto
Copy link
Contributor Author

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.

@arueckauer
Copy link
Contributor

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.

Base automatically changed from master to main January 28, 2021 19:16
@neenjaw neenjaw added the stale label Jun 18, 2021
@github-actions github-actions bot removed the stale label Jul 1, 2021
@dstockto
Copy link
Contributor Author

Closing this. I think much of what this was trying to accomplish has been handled elsewhere and more recently.

@dstockto dstockto closed this Sep 29, 2021
@dstockto dstockto deleted the accumulate-namespace branch September 29, 2021 03:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants