Skip to content

Conversation

@cxw42
Copy link
Contributor

@cxw42 cxw42 commented Oct 3, 2019

This is my first try at one of these, so please let me know what changes I may need to make! I tested it with

carton exec bin/exercise-gen.pl robot-name && prove exercises/robot-name/.meta/solutions/

and it passes all tests. I used the bob and pig-latin conversions as examples.

I added a perlmaven link to the stub in case students need help getting started with Moo. I am not affiliated with Gabor in any way, but I have found his resources useful in the past. I am happy to change that if you have a better link.

Thank you for considering this PR!

-Chris (CXW)

Edit I should also add that I am working in cygwin and couldn't get configlet to work. Therefore, I have not updated the README. Please let me know if that is essential, and I'll see what I can hack.

@cxw42 cxw42 mentioned this pull request Oct 3, 2019
62 tasks
@m-dango m-dango self-requested a review October 3, 2019 11:17
@m-dango
Copy link
Member

m-dango commented Oct 3, 2019

Thanks for doing this! clock may also be a good one to look at as that also uses Moo.

We should have a cpanfile in this exercise directory so the user gets it with the exercise. I'd suggest copying the one that clock uses as it has a range of OOP options to choose from.

@m-dango
Copy link
Member

m-dango commented Oct 3, 2019

re: the configlet, it looks like there aren't any changes to the readme for this exercise so don't worry about it. I've not used cygwin myself so sadly I'm not too sure what to suggest to get it working. The configlet comes from https://github.com/exercism/configlet if you want to take a look but I don't think there are any major readme changes at the moment.

@cxw42
Copy link
Contributor Author

cxw42 commented Oct 4, 2019

Re. the README - Thanks! I can probably run it under Strawberry if necessary in the future. I cleaned up some whitespace by hand.

While cleaning, I noticed that the README says:

The names must be random: they should not follow a predictable sequence. Random names means a risk of collisions. Your solution must ensure that every existing robot has a unique name.

The existing version of robot-name does not guarantee that, and doing so would require both BUILD and DEMOLISH (as far as I know). Would you like me to edit the README or the code?

@cxw42 cxw42 force-pushed the robot-name branch 2 times, most recently from db72a5a to bbd4224 Compare October 4, 2019 01:41
@m-dango
Copy link
Member

m-dango commented Oct 4, 2019

It'd be nice to have, but I'd also be happy to address that in a separate PR with improvements to the test to better check no duplicates can happen.

The perl6 implementation goes through every possible name so it could be worth looking there for ideas: https://github.com/exercism/perl6/tree/master/exercises/robot-name

@m-dango
Copy link
Member

m-dango commented Oct 4, 2019

For the readme, it'd probably be better to leave it as is. The readme is generated with a combination of config/exercise_readme.go.tmpl and the problem-specification descriptions with the configlet. These files should be changed instead as otherwise changes made manually would be reverted by the next person to run the generator on the exercise.

@cxw42
Copy link
Contributor Author

cxw42 commented Oct 5, 2019

For the readme, it'd probably be better to leave it as is.

I submitted a PR to the problem-specifications repo for the readme changes. Thanks for the tip! Edit Commit d283041 puts the README back the way it was.

no duplicates

I agree with the idea of a separate PR. I think that would make the commit history easier to follow.

Edit 9cb26db is rebased on master. I think this is ready --- please let me know if I am missing something. Thank you!

@m-dango m-dango merged commit 4e5665b into exercism:master Oct 5, 2019
@cxw42 cxw42 deleted the robot-name branch October 5, 2019 12:58
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.

2 participants