Sum-of-multiples: Add information to the description. #961
Conversation
rpottsoh
changed the title
|
I've edited the OP to make clear it's referencing a ticket in another repository. I was confused at first when I followed the link... |
| @@ -5,3 +5,5 @@ If we list all the natural numbers up to but not including 20 that are | |||
| multiples of either 3 or 5, we get 3, 5, 6 and 9, 10, 12, 15, and 18. | |||
|
|
|||
| The sum of these multiples is 78. | |||
|
|
|||
| PS: Notice that multiples of both 3 and 5 like for example 15 will counted only once. | |||
There was a problem hiding this comment.
This post scriptum feels out of place (well, thats what post scriptums always do).
I'd prefer to add the word "unique" as a qualifier to the first sentence:
Given a number, find the sum of all the unique multiples of particular numbers up to
but not including that number.
(emphasis to make the difference clear, not to be added into the description)
|
@hekrause: It would be more useful if the description of the problem and the change that is being made to solve that problem were a part of the pull request, rather than requiring reviewers to hunt down a rabbit hole of links to work out what was going on. |
|
@Insti In exercism/rust#360 @petertseng mentioned that the solution to this issue should better be handled in the problem-specification repository and not in the language specific rust repo. So I made one link directly to the issue where the problem is explained. "hunt down a rabbit hole" is a bit much :-) But for future issues and pull request I will keep that in mind. |
|
Even if it is only a single link, a short recap is always a nice thing. |
I apologise, I guess I was a bit cranky earlier, sorry, this was not your fault.
Thanks. ❤️ |
|
all right, all right, for bookkeeping reasons I will Request Changes (in the GitHub sense), though it may be that the requested change is simply to comment "we don't need to keep the two in sync" |
There was a problem hiding this comment.
Should this be added to https://github.com/exercism/problem-specifications/blob/master/exercises/sum-of-multiples/metadata.yml#L2 as well? It doesn't necessarily have to be, but since the sentences are currently the same, if we choose now to make them diverge we should make sure the decision is deliberate.
|
I do not think the blurb in disclaimer: I'm also slightly against the word 'unique' being added to the description. |
|
The Exercism description unhelpfully focuses on the multiples of particular numbers in relation to another unrelated number which is actually the limit of a range.
The Project Euler problem this exercise is based on does not have this issue:
The Exercism blurb and description should be re-written to improve clarity. |
petertseng
dismissed
their stale review
"we should make sure the decision is deliberate". We now have.
| but not including that number. | ||
|
|
||
| If we list all the natural numbers up to but not including 20 that are | ||
| multiples of either 3 or 5, we get 3, 5, 6 and 9, 10, 12, 15, and 18. | ||
| If we list all the natural numbers below 20 that are multiples of 3 or 5, we get 3, 5, 6 and |
There was a problem hiding this comment.
I think the "and" after 6 should be removed:
"multiples of either 3 or 5, we get 3, 5, 6, 9, 10, 12, 15, and 18."
| but not including that number. | ||
|
|
||
| If we list all the natural numbers up to but not including 20 that are | ||
| multiples of either 3 or 5, we get 3, 5, 6 and 9, 10, 12, 15, and 18. | ||
| If we list all the natural numbers below 20 that are multiples of 3 or 5, we get 3, 5, 6, 9, 10, 12, 15, and 18. |
There was a problem hiding this comment.
I've just remembered that we should break lines at 80 columns. Could you perhaps fix that?
|
Merged. Thanks @hekrause! |
Changes for Issue exercism/rust#360