exercism / python

Exercism exercises in Python.
https://exercism.org/tracks/python
MIT License
1.94k stars 1.29k forks source link

Clock exercise goes against documented use of `__repr__` method #2813

Closed Chris-May closed 2 years ago

Chris-May commented 2 years ago

This issue isn't the biggest of deals, but the python documentation about __repr__ suggests that:

If at all possible, [the string returned] should look like a valid Python expression that could be used to recreate an object with the same value (given an appropriate environment). If this is not possible, a string of the form <...some useful description...> should be returned.

I would imagine the reason the exercise suggests defining the __repr__ is that every time the student sees errors from the test cases, they'll see the hh:mm repr, as opposed to the <clock.Clock object at 0x102247190> repr.

Would there be an appetite to handle both methods? Or am I complicating things?

I like that this exercise is an excellent example to show why they might want to implement both __str__ and __repr__.

github-actions[bot] commented 2 years ago

🤖   🤖

Hi! 👋🏽 👋 Welcome to the Exercism Python Repo!

Thank you for opening an issue! 🐍  🌈 ✨


​          ◦ If you'd also like to make a PR to fix the issue, please have a quick look at the Pull Requests doc.
             We  💙  PRs that follow our Exercism & Track contributing guidelines!


💛  💙  While you are here... If you decide to help out with other open issues, you have our gratitude 🙌 🙌🏽.
Anything tagged with [help wanted] and without [Claimed] is up for grabs.
Comment on the issue and we will reserve it for you. 🌈 ✨

BethanyG commented 2 years ago

Hi @Chris-May 👋🏽

Thanks for filing this issue. 😄

Would there be an appetite to handle both methods? Or am I complicating things?

You are complicating things -- but in the best way. Because there isn't a whole lot of straightforward guidance or examples for overriding or customizing these methods. Worse yet - many libraries and projects don't use these quite correctly either. I remember coming across the Stack Overflow post below at some point, and thinking "I am just going to ignore these two methods, and hope my problem goes away..."   😆  . Of course, it didn't.

Stack Overflow: __str__ vs. __repr__ And within the post is this excellent rundown from Alex Martelli: Not a Useful Default

So -- TL; DR I think it's a great idea, and I'd love to see if we could add in __str__ vs __repr__ for this exercise. And myself and @J08K would love to see a PR from you around the topic (if you are willing).

But now for the pecked to death by ducks part ---

Implementation Details
This exercise is a [**practice**](https://github.com/exercism/docs/blob/main/building/tracks/practice-exercises.md#files) exercise. And that means that the instructions and the tests are auto-generated from the central [problem-specifications](https://github.com/exercism/problem-specifications/tree/main/exercises/clock) repo. Since your proposed change would be Python-specific, we need to do _appends_ to the instructions and tests rather than propose spec changes or re-writes. It also means we may need to alter the JinJa2 template that's used to generate the test file. Here are two examples of "Instruction Appends" files. These get concatenated to the end of the canonical exercise instructions: - [High Scores Append](https://github.com/exercism/python/blob/main/exercises/practice/high-scores/.docs/instructions.append.md) (_this one is appended without a header - the append header is stripped_) - [Circular Buffer Append](https://github.com/exercism/python/blob/main/exercises/practice/circular-buffer/.docs/instructions.append.md) (_this one has a header_) For additional tests, we define a `additional_tests.json` file. An example from the `wordy` exercise is below: - [`wordy` additional_tests](https://github.com/exercism/python/blob/main/exercises/practice/wordy/.meta/additional_tests.json) The existing tests and the additional tests get merged to generate the test file via a JinJa2 template. Here is `clock`s template: - [`clock` JinJa2 template](https://github.com/exercism/python/blob/main/exercises/practice/clock/.meta/template.j2)

For reference, here is the clock exercise directory: Clock Exercise Directory

I think my general thoughts would be to keep the instruction_append as minimal as you can without sacrificing clarity, but that it would also be very useful to set up the __repr__ vs __str__ issue and provide a couple of links and some code examples -akin to what is done for error-handlng in the circular-buffer exercise. We don't (yet!) have a concept exercise that covers either classes or dunder methods, so some instruction/example here is probably necessary.

Again - I think its a great idea. Let @J08K or myself know if you are interested in working on this -- and if you need help decoding the docs, or anything else! 🌟 😄

Chris-May commented 2 years ago

Awesome! I would love to help out with this.

In addition, I'd be interested in helping to create one or more dunder-method exercises. I'm writing a book on classes in python, and I'm thinking about a few anyway. It would be nice to have more of the topic covered in Exercism!

BethanyG commented 2 years ago

Hi @Chris-May -- apologies for the delay in messaging back. I've assigned you this issue, and I'm delighted you want to take this on!

I would love to chat further about dunder-method exercises -- both our existing practice exercises, and the class related concept exercises we're about to add. We have a lot of exercises you could potentially work on, should you have the interest and time! 😄

Feel free to reach out to either @J08K or myself if you have questions, issues, or would like to brainstorm ideas. 🌟

Chris-May commented 2 years ago

That sounds great, @BethanyG! I would be happy to help out as I can!

I've added an initial pull request for you to review. I'm not confident I updated everything I needed to, and I am very much in favor of editing work or any other suggestions you may have for... well, anything.

Thanks for your work!

Oh! In addition, I would love to know what other exercises you'd like help with!

BethanyG commented 2 years ago

As #2829 is now merged and closed, closing this related issue. Please feel free to re-open should you feel the need. 😄