Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Pascals Triangle] Remastered for recursion #3150

Merged
merged 27 commits into from
Dec 10, 2022

Conversation

PaulT89
Copy link
Contributor

@PaulT89 PaulT89 commented Jul 24, 2022

As per Issue #3078.

Changes

Rewrote example.py:

  • Uses recursion rather than loops.
  • Raises a meaningful error when row_count < 0, instead of just returning None.

Added template.j2:

  • Based general layout of the template on the old pascals_triangle_test file.

Added instructions.append.md:

  • Encourage student to use recursion to solve this problem
  • Added boilerplate message about how to raise exceptions

.meta/config.json:

  • Added name as co-author (though maybe contributor would be more appropriate?)

config.json:

  • practices recursion
  • added (seemingly) sensible sounding prerequisites
  • kept difficulty
  • moved entire block up to be in line with other exercises of similar difficulty.

Rewrote example.py:
- Uses recursion rather than loops.
- Raises a meaningful error when row_count < 0, instead of just returning None.

Added template.j2:
- Based general layout of the template on the old pascals_triangle_test file.

Added instructions.append.md:
- Encourage student to use recursion to solve this problem
- Added boilerplate message about how to raise exceptions

.meta/config.json:
- Added name as co-author (though maybe contributor would be more appropriate?)

config.json:
- practices recursion
- added (seemingly) sensible sounding prerequisites
- kept difficulty
- status = wip for now
- moved entire block up to be in line with other exercises of similar difficulty.
Rewrote example.py:
- Uses recursion rather than loops.
- Raises a meaningful error when row_count < 0, instead of just returning None.

Added template.j2:
- Based general layout of the template on the old pascals_triangle_test file.

Added instructions.append.md:
- Encourage student to use recursion to solve this problem
- Added boilerplate message about how to raise exceptions

.meta/config.json:
- Added name as co-author (though maybe contributor would be more appropriate?)

config.json:
- practices recursion
- added (seemingly) sensible sounding prerequisites
- kept difficulty
- moved entire block up to be in line with other exercises of similar difficulty.
@github-actions

This comment was marked as resolved.

@BethanyG BethanyG self-requested a review July 24, 2022 11:12
@BethanyG
Copy link
Member

Hi @PaulT89 👋🏽

Thank you so much for submitting this! Jus to warn you: I will probably be taking this in stages, since my time today/next week is a bit limited. I'll move as quickly as I can, but you may get several "waves" or clusters of comments. At a quick first glance:

  1. This looks to be missing the ./meta/tests.toml file (see this one from diffie-hellman as an example). The tests.toml file is needed by the generator so that it can figure out which test cases to generate/not generate for the exercise. I think the file can be created by using configlet, but you may need to dig in the practice exercise docs for details (apologies - I don't have time at the moment to go dig for a link).
  2. Instead of putting additional tests in the JinJa2 template, we currently define them in a ./meta/additional_tests.json file. See this one for the clock exercise as an example. Seems tedious for just one test case, but that's the way we track them.
  3. If the error handling case is from cannonical data, then you don't have to make an additional_tests.json file -- but you should also change the comment in the template, so that it doesn't confuse us in the future with implying that there are "additional" test cases for this exercise.
  4. Making the recursion concept a prerequisite for this exercise means that a student in learning mode won't be able to unlock this exercise without completing the recursion concept exercise. But we don't currently have a recursion concept exercise...so this exercise would never be unlocked. 😱 Suggest we remove the prerequisite until we can get a recursion concept exercise completed. (are you up for a challenge? Wanna take on a recursion concept exercise? Just let me know.... 😄 )
  5. You totally should be an author here! The JinJa2 template alone is worth an author credit, but you also thought through error handling and other changes.
  6. We may want to elaborate a little bit on recursion in the instruction append. I don't have time at the moment, but there is a way to cross-link concept docs inside instructions. I think we should do that here. We may also want to do an additional callout/warning for Python's recursion limit, so that students coming from a language that doesn't limit recursion aren't caught by surprise. Ditto for the lack of tail-call optimization.
  7. We might want to consider a hints.md file here to give students some extra links around thinking through recursion. It might also be useful to have some hints on how to do this via for or wile loop ... just to cover our bases. But we can discuss that later on.

Apologies -- that's all I have for now, but will pick this up later today my time. Thanks again for all your hard work on this! 🎉 🌈

@PaulT89
Copy link
Contributor Author

PaulT89 commented Jul 25, 2022

  1. The .meta/tests.toml file already exists, but remains unmodified (which is why it doesn't show up in this PR).
  2. Okay, I'll go back and fix that. FIXED
  3. There's no test for negative rows in the canonical data. The old pascals_triangle_test.py file tested for negative input, but it checked that None was returned, and I thought that it would be nice to keep the test, but raise a meaningful exception message instead.
  4. Hmm... 🤔. You're right, that does seem somewhat counterproductive - I should probably fix that. FIXED As for creating the concept exercise, maybe later (like in a few weeks?) if nobody else has taken it (and I haven't got anything else on). In theory at least, Pascal's Triangle could become the concept exercise.
  5. YAY! 🎉
  6. Yeah, I kept the instructions to a minimum because I imagined the concept exercise as already existing. I forgot that we don't live in the Platonic Ideal Universe inside my head, so that's an oops on my part. I'll go back and add a bit more to the instructions.
  7. Re: hints.md - yeah, maybe. Re: completing with for and while loops - I doubt that'll be an issue, though I suppose it would depend on how early/late the Recursion concept is put in the track.

Just one more thing. You mentioned Python's recursion limit in point 6 (something that I completely forgot about), but it gave me an idea. I've been trying to figure out a way to ensure that the student completes the exercise using recursion instead of loops. So far, the best that I could come up with was to simply rely on the student to complete this exercise in good faith (i.e. "I totally did it with recursion, not loops - trust me 😜"). But what if there's an additional test that deliberately uses Python's recursion limit to test for this? DONE Maybe something like this:

import sys

def test_solution_is_recursive(self):
    with self.assertRaises(RecursionError) as err:
        rows(sys.getrecursionlimit() + 10) # +10 just to be sure
    self.assertEqual(type(err.exception), RecursionError)
    self.assertEqual(err.exception.args[0], "maximum recursion depth exceeded in comparison")

-Fixed config prerequisite bug (exercise is now accessible).
- Moved hardcoded additional tests to additional_tests.json file.
- Added test for recursion.
@PaulT89 PaulT89 marked this pull request as draft July 25, 2022 13:36
@PaulT89
Copy link
Contributor Author

PaulT89 commented Jul 25, 2022

Instructions update plus additional hints still pending. Can't seem to figure out why Exercises check / cannonical_sync 3.6, 3.7, and 3.8 keep failing. I might need to download Python 3.6 to figure that out.

Apparently the RecursionError message changed in Python 3.9.  Hopefully this will work across all tested versions now.
@PaulT89
Copy link
Contributor Author

PaulT89 commented Jul 26, 2022

Success! Well that's a fun/annoying little gotcha - the RecursionError message seems to be slightly different across versions (though at least the first 32 chars are identical).

@PaulT89
Copy link
Contributor Author

PaulT89 commented Jul 30, 2022

Okay, I've updated the instructions and added hints. It still might be a little too bare-bones, but I'll leave the final assessment up to you.

@PaulT89 PaulT89 marked this pull request as ready for review July 30, 2022 13:38
@BethanyG BethanyG self-assigned this Aug 26, 2022
Rewrote example.py:
- Uses recursion rather than loops.
- Raises a meaningful error when row_count < 0, instead of just returning None.

Added template.j2:
- Based general layout of the template on the old pascals_triangle_test file.

Added instructions.append.md:
- Encourage student to use recursion to solve this problem
- Added boilerplate message about how to raise exceptions

.meta/config.json:
- Added name as co-author (though maybe contributor would be more appropriate?)

config.json:
- practices recursion
- added (seemingly) sensible sounding prerequisites
- kept difficulty
- moved entire block up to be in line with other exercises of similar difficulty.
Rewrote example.py:
- Uses recursion rather than loops.
- Raises a meaningful error when row_count < 0, instead of just returning None.

Added template.j2:
- Based general layout of the template on the old pascals_triangle_test file.

Added instructions.append.md:
- Encourage student to use recursion to solve this problem
- Added boilerplate message about how to raise exceptions

.meta/config.json:
- Added name as co-author (though maybe contributor would be more appropriate?)

config.json:
- practices recursion
- added (seemingly) sensible sounding prerequisites
- kept difficulty
- status = wip for now
- moved entire block up to be in line with other exercises of similar difficulty.
-Fixed config prerequisite bug (exercise is now accessible).
- Moved hardcoded additional tests to additional_tests.json file.
- Added test for recursion.
Moved import sys
Apparently the RecursionError message changed in Python 3.9.  Hopefully this will work across all tested versions now.
-Updated Instructions.append.md
-Added hints.md
@PaulT89 PaulT89 marked this pull request as draft October 13, 2022 11:31
@BethanyG
Copy link
Member

Gah! 😱 Apologies that this has been sitting so long, and has now incurred the wrath of the "did-not-updated-test-cases" bot. @PaulT89 -- I am pretty swamped this morning, but I am hoping to get to this by the end of my day today (PDT), and get it un-stuck and on its way. Thank you so so much for your patience.

@PaulT89
Copy link
Contributor Author

PaulT89 commented Oct 13, 2022

@BethanyG Yeah, well, it's been a while since I've done anything on Github, so I decided to update all my repos and rebase various branches - and this happened. I think the CI is objecting to differences in the prob-specs vs the actual tests, so I regenerated and added a PR with the changes. Once that goes through, I'm assuming another rebase will fix this.

@BethanyG
Copy link
Member

BethanyG commented Oct 14, 2022

@PaulT89 - indeed it did! Thanks again for doing that. I went ahead and rebased your branch, so now the test are passing. 🎉 I won't have time until the weekend to review the PR. But I will definitely do it then. This has sat an obscenely long time, and is well past a push to production. 😉 My apologies for that. Life and a new job completely swamped me.

I think you (or I) will end up with one more rebase -- there is a PR from Katrina that I will probably approve and merge tonight, so her changes will need to be incorporated, I think.

@PaulT89 PaulT89 marked this pull request as ready for review October 15, 2022 07:29
PaulT89 and others added 2 commits November 27, 2022 18:54
Shortened the JinJa Template a bit.
Edited instruction append to use reflinks and a recursion widget, and be one sentence per line.
Retested everything.
@BethanyG BethanyG added the x:rep/large Large amount of reputation label Dec 10, 2022
@BethanyG
Copy link
Member

@PaulT89 - I cannot apologize enough for letting this sit. I've made a few edits to shorten the JinJa template and put in reflinks & widget links into the instruction append. Everything's been re-tested and looks good.

THANK YOU for this exercise! It is approved, and I am merging it in. 😄 I've also given you a little rep bump for your patience and efforts -- this one took way too long.

config.json Outdated Show resolved Hide resolved
config.json Outdated Show resolved Hide resolved
Copy link
Member

@BethanyG BethanyG left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent work, @PaulT89! Thank you again for working on this. 🌟

@BethanyG BethanyG merged commit c5da497 into exercism:main Dec 10, 2022
@PaulT89
Copy link
Contributor Author

PaulT89 commented Dec 11, 2022

@PaulT89 - I cannot apologize enough for letting this sit. I've made a few edits to shorten the JinJa template and put in reflinks & widget links into the instruction append. Everything's been re-tested and looks good.

THANK YOU for this exercise! It is approved, and I am merging it in. 😄 I've also given you a little rep bump for your patience and efforts -- this one took way too long.

That's okay. I deliberately didn't push the issue - it was funnier that way. The longer it took for you to realize that this wasn't merged, the funnier it became. Consider me suitably amused. 🤣

@PaulT89 PaulT89 deleted the pascals-triangle-branch branch December 11, 2022 07:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
x:rep/large Large amount of reputation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants