raindrops: include edge cases in tests

[McEileen]

I added new tests for edge cases after opening issue #434 on the Ruby track. exercism/ruby#434

[kytrinyx]

This is great, thank you.

Would you mind updating the commit message to explain how those edge cases were being hit before?

Also, we have reorganized the files—would you mind rebasing this onto master?

[petertseng]

If I could make a small request - I would like it if the whitespace changes are in a separate commit than the new test case additions. The reason is so that someone who wants to just understand what new test cases are added can just look at that commit, and not be distracted by the whitespace changes.

[kotp]

Should test 27 be the simpler case? Or at least be included? If that is passing, then the power of 5 and power of 7 may also be caught.

For history on this, see the discussion on exercism.

[McEileen]

Hi, I changed the commit message on my local and also put the spacing changes (which were caused by adding new tests) in another commit. I'd be happy to include 27 as a test case, too.

However, when I type git rebase upstream master it shows fatal: Needed a single revision and then invalid upstream upstream

I'm not sure what's causing this? Typing git remote -v returns

origin https://github.com/McEileen/x-common.git (fetch)
origin https://github.com/McEileen/x-common.git (push)
upstream https://github.com/exercism/x-common.git (fetch)
upstream https://github.com/exercism/x-common.git (push)

Help appreciated, thanks!

[petertseng]

The remote looks correct to me. Have you fetched from exercism's copy of the repo with git fetch upstream yet?

I believe you will want to run git rebase upstream/master raindrops-edge-cases rather than git rebase upstream master. When running git rebase A B:

• A is the branch you want to rebase off of. It can't be just upstream because upstream is a remote, not a branch. Therefore, upstream/master is correct.
• B is optional (defaults to current branch) but if it is specified it's the branch whose commits you want to rebase. Here, that's raindrops-edge-cases, as I see from the top of this PR ("McEileen wants to merge 7 commits into exercism:master from McEileen:raindrops-edge-cases").

[petertseng]

The commits a82a70c and 554c929 look good, and it would be good if those were the only two commits in this PR (as seen at https://github.com/exercism/x-common/pull/370/commits ) instead of including unrelated commits such as Erik's.

If all else fails, you could:

git checkout raindrops-edge-cases
git reset --hard upstream/master
git cherry-pick a82a70c78ee48262766e9651cc1e13d6a6db72dc
git cherry-pick 554c9293958b0642c6e2081c72ff28cba1bfa08e
git push -f

[nickborromeo]

@McEileen I noticed that your commits are not associated to your user. You can take a look at commits are not linked to any user. Hope this helps 😄

[McEileen]

@nickborromeo Very helpful link, thanks! Strangely, when I clicked on the individual commits, it now shows my icon as a small icon in the lower right corner of the grey octocat icon, and it says Eileen McFarland committed with McEileen 5 days ago There's also no question mark to hover on to see why the commit wasn't linked. Not sure what happened

I'm going to squash all my commits into two commits now(the test cases and spacing changes), which I hope will resolve the issue.

[petertseng]

Personally I think this looks good!

One question I would like to ask of everyone: Is the purpose of the tests such as 8 (2^3), 27 (3^3), 3125 (5^5) clear from just looking at the file and/or factorising the number? If it is not, maybe add a comment via "comment": "something" or "#": "something".

If nobody has anything to say in 48 hours I'll merge by default.

[kotp]

I am wondering how testing for 0 would be communicative, in that it is not prime, and is a factor of any number, and a test for it resulting in the translations in order would ensure that is covered as well.

Two commits rather than one may not be necessary.

[petertseng]

Two commits rather than one may not be necessary.

Having the whitespace in the same commit as the additional tests is distracting to reviewers. I would very much like them to stay as two commits, please.

Edit: I do realize I am stating a personal preference here, since all I can say for sure is that it is distracting to me, and not whether it will be distracting to all reviewers. I do speak truthfully about what I know about myself, though. And this is in keeping with the principle of having one logical change per commit (a la https://www.kernel.org/doc/Documentation/SubmittingPatches "3) Separate your changes.")

[petertseng]

zero is indeed an interesting case. If we are just going for divisibility, it is divisible by all the integers (well, not itself). If we think about it in terms of factors, would we also say that zero has all integers as factors?

Testing that the order is right has already been done by 105 though, so adding zero would just to be demonstrate the properties of zero, get people to think mathematically. Is it something we want?

[rbasso]

Considering that there are two possible results for zero, testing it would push people toward a more restricted set of implementations.

If we don't test it, we get more diversity!

[petertseng]

Sounds good. Let us take this as is. If we do change our mind about zero, that can always come in a separate PR of course! Or tracks can note a specific exception they wished to make.

[kytrinyx]

Having the whitespace in the same commit as the additional tests is distracting to reviewers. I would very much like them to stay as two commits, please.

I just wanted to add my voice to this: I like having formatting changes separately. This also goes for refactoring vs new changes.

[jtigger]

Did anyone notice what fantastic teamwork went down in the PR? This is a good example of what I love about this community. :)

[McEileen]

Thanks for all the helpful feedback! With the exception of correcting typos, this was the first PR I've made to exercism, and I appreciated all the help.