[Clock] Improved/additional tests for __add__ and __sub__

[IsaacG]

Motivation: I've seem multiple solutions where Clock.__add__ either (1) returns a str or (2) modifies and returns self. Ideally a = b + c should not mutate b or c and Clock + int should return a Clock.

Proposal: Add a test or two where,

now = Clock(0, 0)
future = now + 5
test.assertNotEqual(now, future)
test.assertEqual(future, Clock(0, 5))

Possibly replicate for __sub__ as well.

I'd be happy to work on this myself.

[BethanyG]

Hi @IsaacG 👋🏽

Thanks for filing this issue. Since the clock exercise pulls from problem-specifications, and already has 100 test cases and three additional test cases for __repr__ tacked on, I am loath to add any more to it by extending additional_tests.json. There are about 50 more tests than there need to be for this exercise already.

The additional_tests.json were intended to only support adding a __repr__ method that was unique to Python. I think what you are proposing here is more changing the way the already existing add and sub test cases are implemented.

To that end, the test generation template, example solution, and generated test files would need to be altered. You might also have to change the test toml file.

But I question if we really want to change the cases. The cannoncial data clearly shows that the result of add and sub are strings. Additionally, there are 2000+ published community solutions that could be invalidated. Since the main point of this exercise was to get students familiar with further customizing classes with things like dunder methods, it feels like a high price to pay to make a few points about mutability.

Rather than going back over this exercise again, I'd rather develop one that would set up and get at the pitfalls of mutability. Overloading this one doesn't feel like a good strategy.

In the past, High Scores was a class based exercise, and has a clear setup around mutability, so that might be a good candidate to turn back into something OOP. We do have an issue open around that one already. And yes - it also has thousands of community solutions. I feel a little less guilty about changing that one, since we radically changed it already, and it is a fairly toothless exercise as it stands.

In fact, either altering High Scores or making an additional OOP version that gets at mutability issues would be a very welcome addition, should you want to take that on. We don't have a lot of practice exercises that really dig into why you would set up a class in a specific way, or when (or when not to) mutate attributes/return altered objects/return new objects.

[BethanyG]

Another exercise that could be "riffed on" or extended is matrix. The current exercise doesn't get at altering or updating the Matrix with new information. But we could either do an addendum or a follow-on exercise that requires that, which could set up the problem of mutating or returning a new object.

[BethanyG]

Adding another note here. this issue gets at a somewhat related issue. Although the opposite of what is described here -- in issue #2926, mutation (and the mutation being reflected in the __repr__ is being discussed. ).

[IsaacG]

Thank you for your response, Bathany. I filed an issue against the problem specs to discuss if changing the canonical expectation around Clock's add and subtract make sense.

Having been part of the discussion on multiple discussions around Matrix updates, I'm hesitant to suggest that it be extended to support mutability. Doing so in a clean fashion would mean having row and column return a bespoke view-like class.

Updating High Scores to address mutability sounds good to me and something I'd be willing to tackle.

• Should discussion about updating High Scores to cover mutability be in this Issue or elsewhere?
• How do you feel about simply testing if those functions mutate the data without introducing a class vs adding a class to the exercise? Adding a test to check if the functions mutate the data ought to be relatively simple and straight forward.

[BethanyG]

Should discussion about updating High Scores to cover mutability be in this Issue or elsewhere?

Elsewhere. Maybe we do it in 2786, since that's the last filed issue against High Scores, and does discuss mutability. That way, the author of that issue will also get a ping letting him know you've picked it up - in case he wants to join in the discussion.

I think (for now) we close this issue -- and also the other Clock issue. We may want to revisit this exercise, but not right now. I feel like it's been mucked with enough for the time being.

How do you feel about simply testing if those functions mutate the data without introducing a class vs adding a class to the exercise?

Thats what the original class-based High Scores did, and you can see that in the tests.toml file. When it was re-written to be function-only, it ... got funky. See #2786 for what happened.

Adding a test to check if the functions mutate the data ought to be relatively simple and straight forward.

This may be a case where simply re-instituting the exercise as it was in 2018/2019 will do the trick, and then altering the test generation template to import the class instead of functions, and changing the tests.toml file to re-activate the tests cases. Since we ported repos for V3, I need to dig for that state. So there may not be any work beyond that for a base state.

But I think we should also take a look at the point you were making with Clock, and ask if that same point could be made here with a "ScoreKeeping" class. There is the question of mutating data - but there is also the questions of mutating state, and the issues and considerations around that. This might be a good opportunity to revisit what we introduce in the class concept exercise, where we have a class attribute and also instance attributes.

I will also copy this comment over to issue #2786, and then close this.