-
-
Notifications
You must be signed in to change notification settings - Fork 695
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
Remove starter implementations #681
Remove starter implementations #681
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 taking this on.
You should delete the entire file that has the starter implementation instead of just deleting the method stubs. Once you delete the file with the starter implementation, add a .keep
file in it's place.
Also, tests don't need to be commented out.
Good luck 😜
@@ -1,21 +0,0 @@ | |||
public enum Plant { |
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.
I'm pretty sure this file needs to stay here as it is needed to complete the exercise. Thoughts @FridaTveit ?
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.
Yes, this should still be here. When we use enums we usually include this in the starter implementation regardless of the difficulty of the exercise. This is because they're not really part of the solution to the problem, they're more part of the problem specification if that makes sense :) Sorry for the confusion @redshirt4! :)
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.
Thank you for helping with this @redshirt4! :) A few changes needed, but that's mostly because we changed out mind about starter implementations just now! Sorry about that :)
@@ -1,21 +0,0 @@ | |||
public enum Plant { |
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.
Yes, this should still be here. When we use enums we usually include this in the starter implementation regardless of the difficulty of the exercise. This is because they're not really part of the solution to the problem, they're more part of the problem specification if that makes sense :) Sorry for the confusion @redshirt4! :)
@@ -23,43 +23,43 @@ public void setUp() { | |||
|
|||
@Test | |||
public void testZeroStepsRequiredWhenStartingFrom1() { | |||
assertEquals(0, collatzCalculator.computeStepCount(1)); | |||
// assertEquals(0, collatzCalculator.computeStepCount(1)); |
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.
As per the discussion in PR #683, we decided not to comment out tests after all as this might be confusing to people solving the exercise. The POLICIES doc has been updated to reflect this. So all you have to do is to remove the CollatzCalculator.java
file completely and replace it with a .keep
file (the .keep
file basically tells git not to ignore that directory even though it's empty). Sorry for the confusion! :)
Thank you @Smarticles101 and @FridaTveit for the clarifications. I'll take a look at the new spec and push. |
121dcd8
to
9629a4b
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.
These look good now, thanks :)
Thank you for the review @Smarticles101 and @FridaTveit . Is it best practice to ask for a re-review of current, or to open a new pull request? |
@redshirt4 I think it is best practice to ask for a re-review or to wait for another review. |
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.
Looks good now
@@ -3,6 +3,6 @@ | |||
|
|||
public class Etl { | |||
public Map<String, Integer> transform(Map<Integer, List<String>> old) { | |||
return null; | |||
throw new UnsupportedOperationException("Delete this statement and write your own implementation."); |
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.
The fact that you didn't delete this starter implementation because it had a complex stub shows that you read and paid attention to the policies doc which means alot
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.
This is looking really good! Thank you for tackling all of these @redshirt4! There's just one file which shouldn't be removed and then you're good to go :) Great work!
@@ -1,31 +0,0 @@ | |||
class MatrixCoordinate { |
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.
Please don't remove this file. Like the occasional enum definitions and exception classes in other exercises, this has been provided as part of the solution so that the exercise isn't overly complicated to implement :) You also don't need a .keep
file if you keep this in.
Sorry @Smarticles101, I didn't notice you were reviewing this at the same time! I was trying to do the review on a train which only occasionally has internet :P I don't think it matters very much about |
@FridaTveit I agree. I missed that file on accident. But yes, it should be kept. (You also double posted, so I deleted the second one) |
- Classes as a whole removed, not just method/constructor stubs - Commenting out tests undone per discussion, exercism#683 - Enum 'Plant' re-added to Kindergarten Garden per discussion - .keep added to empty source folders
…ies. bob, pascals triangle correct as is. bracket push stubs removed
…s. beer song stubs removed.
… with starter throw.
…enum and very specific Grid Position with non-standard equals, leaving those in while removing other stub.
…ption in place. anagram stubs removed
742cd6a
to
e8c52fb
Compare
@FridaTveit changes pushed to address review kickbacks |
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.
I went over everything and it seems like you got everything in order and it's all how it should be, so I'd say it's ready to merge. Thanks for all the effort you put into this @redshirt4 😄
Began implementing changes for Issue #554, currently incomplete.
I would appreciate an in-progress review to ensure implementation matches coding standard. I tried to follow the spirit of the starter implementation policy, but am unsure if empty classes are considered a stub.
Reviewer Resources:
Track Policies