perfect-numbers: Update perfect-numbers to reflect changes to canonical data

[emerali]

Resolves #1001

A few things to note:

• I added errors + error catching to both the example solution and the test cases (as these were specified in the canonical data)
• I refer to the range of valid inputs as "positive whole numbers" as the definition of natural numbers can either include or exclude 0. If this is accepted, I'll open an appropriate PR to update the canonical data accordingly
• I've put two blank lines below the version string as my PEP8 linter was complaining about it (it's possible my linter is not configured properly)

[cmccandless]

By convention we use context manager syntax for assertRaises. Example:

with self.assertRaises(ValueError):
    classify(0)

Also, if the canonical data specifies an error string, it is best practice to use assertRaisesRegexp to check that the raised error contains the correct reason.

[emerali]

I've made the requested changes. However, it seems like assertRaisesRegexp is deprecated under Python 3 as it was renamed to assertRaisesRegex in 3.2.

Is this alright?

[cmccandless]

@emerali I was not aware of the deprecation. In order to maintain backwards compatibility with Python 2.7, we have the following options:

• Revert to assertRaises
• Keep assertRaisesRegexp, but add a comment acknowledging deprecation and explaining why it was used anyway
• Use the following:

import sys
if sys.version_info[0] == 2:
    with self.assertRaisesRegexp(
                ValueError,
                ("Classification is only possible"
                 " for positive whole numbers.")):
            classify(0)
else:
    with self.assertRaisesRegex(
                ValueError,
                ("Classification is only possible"
                 " for positive whole numbers.")):
            classify(0)

@N-Parsons @m-a-ge thoughts?

[ilya-khadykin]

@cmccandless checking the Python version looks reasonable to me

[N-Parsons]

@cmccandless, I would prefer that we just use assertRaises - do we really care about the exact wording that someone is raising within their ValueError?

If we do want to continue with checking the message, then I would suggest defining a new class method that will call either self.assertRaisesRegexp or self.assertRaisesRegex dependent on version. This could be done (inside the class) with:

try:
    assertRaisesMessage = self.assertRaisesRegex  # Python3
except AttributeError:
    assertRaisesMessage = self.assertRaisesRegexp  # Python2

If we're going to be testing that messages are passed when exceptions are raised, I think we should be careful about how strict we are with the exact message. Matching an important word might work, or we could just test that there is a non-empty message (ie. ".+") - I think it requires some thought.

[cmccandless]

@N-Parsons the only reason I advised checking the error message is that in this case the spec for this exercise actually contains specific error messages. Normally they do not.

However, I think you raise a good point on exact wording.... Perhaps it would be best to ignore error messages entirely as a track.

[emerali]

Just to add my 2 cents:
I think it'd be better to have there be an exact error message provided in the tests, as this'll give users hints on edge cases to look out for.
Secondly, I think the test cases should be as easy to read as possible, and while I love functional shenanigans, since many users are likely not super familiar with either programming or python (especially true here given how simple this problem is), I think the try-except solution might be a bit hard to understand (though I can't really think of a decent alternative).

[N-Parsons]

@cmccandless, while they do appear in the problem spec, I don't know how important the exact wording actually is (and there's some discussion about how to represent errors in exercism/problem-specifications#905 and exercism/problem-specifications#902). I'm happy with either checking or not checking error messages, but if we are going to expect a particular error message, then it should go into the README (via .meta/hints.md). I think it might be worth raising an issue to discuss error message checking in general, so that we can come to a track-wide decision and keep track of whatever changes might be necessary to keep exercises consistent.

@emerali, expecting exact error messages is ok, but you will need to put a note about it in .meta/hints.md (you'll probably need to create it) and then regenerate/manually update the README to include it. I always find it really frustrating when I have to look in the test-suite to find something like this that is required despite it not making any functional difference.

With regard to using a try .. except block, I think it's the cleanest solution, and it avoids redundant code. I also don't think that we should shy away from using fundamental concepts - people can learn from seeing them, and they aren't required to understand it anyway because it's part of the test-suite and not the exercise.

[cmccandless]

@N-Parsons I've created a new issue to discuss convention on handling exception messages.

[cmccandless]

Pending discussion in #1080

[cmccandless]

@emerali Thank you for your patience on this. I'd say for now remove all checks for exception messages. If the conclusion in #1080 calls for checking them, a new pull request will be made to add them (other exercises will likely require similar changes anyway).

[emerali]

@cmccandless No worries, I got a free t-shirt out of this, so I'm happy 😄

I've removed the exception message checks (and also updated the placeholder file to remove the unnecessary skeleton function), please double check and let me know if I've missed anything.

Once the discussion in #1080 concludes, do let me know if you need some help updating some exercises, I'd be glad to help!

[cmccandless]

@emerali Merging. Congrats, and thanks for working on this!