Skip to content

Add __repr__ functions to chapter5-base tests and make int/float cast consistent

Heads-up! : This MR is kind of long since it required regex'ing a whole bunch of the tests. The key files to look at are the chapter-5-base tests where the doctests have a comment that students can see providing the __repr__ functions to use, and chapter6-exercise-font-tests where a __repr__ that was provided was removed (although it was weird that that __repr__ only appeared there when it was necessary for chapter 5 tests 🤷 )

What is this MR?

This MR provides __repr__ functions to students for chapter 5, and these repr functions cast their arguments to make sure that the output is printed in a consistent way (rather than mixing floats and ints).

Upsides

There are two main benefits to this in my view:

  • The tests are less confusing. Before this there wasn't really a clear reason for why the tests switched between int and float values. I implemented following the book as close as possible, and still didn't match the tests' expectations. This is a silly bug for students to chase since it's arbitrary, inscrutable and implementation specific.
  • This now allows multiple equivalent implementations. Before it would have been an issue if a student maintained that x and y coordinates of BlockLayout objects were integers, now it doesn't matter if it's a float or an integer as it will be cast before being printed and the doctests will accept both.

Potential downsides

  • People who've already passed chapter 5 will need to copy the updated __repr__ functions into their code.
    • I see this as relatively minor considering it's easy, and most people aren't done with ch5 yet.
  • Original ch5 tests may have helped students pass later chapter tests in their current state:
    • Later chapters also have this weird int and float inter-mixing unfortunately. Passing ch5 tests as they are now would mean that your behavior more closely matches what it's expected to be, and thus may make it easier to replicate the specific behavior the tests currently expect.

The second one is more legitimate, but also just a product of... the weird nature of the tests. The solution would be to enforce this float standardization across all tests rn in this MR, but I'm worried about making such a large refactor at once. (Well it's not too bad, but an issue is that I'm not really able to test the changes since I don't have code for chapters all the way up to chapter 10)

Looking Ahead

Looking ahead there are tests in the future chapters for objects which seem similar (objects which also have x, y coordinates etc.), and unfortunately these tests have the same issue where sometimes the value is printed as an integer, and other times as a float. I did not try to modify these yet since I wouldn't be able to test my changes (I don't have code for those yet), and also thankfully they're independent from modifications that needed to be made for this change.

But in the future these should also probably be standardized to expect floats-only where ints and floats are mixed.

Post-Mortem

If I were to do a tl;dr debugging post-mortem: I'd say it's a major code-smell when the tests expect different output formats without a clear reason. Instead the tests should be based on the result of a cast to the overall expected type (here I settled on float).

Random notes (original MR body)

Woo boy, should be done now. At first I tried setting things to ints, but then I realized later chapters have tests where the value truly is a floating point number. Then I thought of modifying the repr function to print using the general format, but the issue with this was that some of the tests were already written to add ".0" when expecting values.

I ended up deciding to just convert all places that Layout/DisplayCommands appear to expect floating point values, and the repr function handles casting to floats. Reasoning: it'll capture all cases where floats are necessary, and also handle ints correctly too.

Edited by Tenzin Low

Merge request reports