Uh oh!
There was an error while loading. Please reload this page.
Add twelve-days practice exercise - #992
Conversation
Hello. Thanks for opening a PR on Exercism 🙂 We ask that all changes to Exercism are discussed on our Community Forum before being opened on GitHub. To enforce this, we automatically close all PRs that are submitted. That doesn't mean your PR is rejected but that we want the initial discussion about it to happen on our forum where a wide range of key contributors across the Exercism ecosystem can weigh in. You can use this link to copy this into a new topic on the forum. If we decide the PR is appropriate, we'll reopen it and continue with it, so please don't delete your local branch. If you're interested in learning more about this auto-responder, please read this blog post. Note: If this PR has been pre-approved, please link back to this PR on the forum thread and a maintainer or staff member will reopen it. |
marcelweikum
commented
Jul 7, 2025
Asking for review #985 (comment)@ahans@vaeng@siebenschlaefer |
alienx5499
commented
Aug 28, 2025
Hi all — could I please get a review on this PR adding the twelve-days practice exercise (addresses #985)? Status:
Would appreciate if a maintainer could review and merge. |
ahans
left a comment
There was a problem hiding this comment.
Thanks for your contribution and sorry for the long wait! Just some comments to get this more in line with the other exercises ...
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
…w for verses, add newlines
…riptions, no newlines in verses, use recite function
…or compile-time constants
alienx5499
commented
Aug 29, 2025
All requested changes have been addressed ✅ |
ahans
left a comment
There was a problem hiding this comment.
Thanks for addressing my comments. However, it appears to me you mostly vibe code this. Please don't do this blindly. It leads to inconsistencies and broken code. Also please test your solution locally first. Thank you for your understanding.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ahans
commented
Aug 29, 2025
I also approved the CI runs now, you'll see that they fail. |
alienx5499
commented
Aug 30, 2025
Thanks for the review! I’ll admit I used AI in some cases to speed things up, but I didn’t vibe code blindly, I tried to follow the review points. I see now that some of those changes introduced issues, so I’ll rework them carefully, clean up formatting, and make sure everything passes before pushing again. |
alienx5499
commented
Aug 30, 2025
When I use constexpr std::string_view expected = ... in tests, I get this linker error: That's why in my latest commit I used std::string expected = ... for the tests to compile and run. |
That is surprising, but is probably due to our ancient version of Catch2. That old version doesn't support string_view properly yet, it seems. The issue here is with Catch2 code that is used to print differences when test cases fail. Anyway, sticking to The reason I advocate for |
ahans
left a comment
There was a problem hiding this comment.
We're getting there! Thanks for you patience!
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
alienx5499
commented
Aug 30, 2025
I have implemented all your suggested changes: template argument deduction for std::array, updated namespace structure, and added const qualifiers to expected variables. All changes committed and pushed successfully. |
ahans
left a comment
There was a problem hiding this comment.
Thanks again for your contribution. I will fix the redundancy in the example and then merge.
| if (start_day == end_day) { | ||
| // Single verse case | ||
| auto result = std::string{"On the "} + | ||
| std::string{ordinals[start_day]} + | ||
| " day of Christmas my true love gave to me: "; | ||
| for (int i = start_day; i >= 1; --i) { | ||
| if (i == start_day) { | ||
| result += std::string{gifts[i]}; | ||
| } else if (i == 1) { | ||
| result += std::string{", and "} + std::string{gifts[i]}; | ||
| } else { | ||
| result += std::string{", "} + std::string{gifts[i]}; | ||
| } | ||
| } | ||
| result += ".\n"; | ||
| return result; | ||
| } else { | ||
| // Multiple verses case | ||
| std::string result = ""; | ||
| for (int i = start_day; i <= end_day; ++i) { | ||
| if (i > start_day) { | ||
| result += "\n"; | ||
| } | ||
| auto verse = std::string{"On the "} + std::string{ordinals[i]} + | ||
| " day of Christmas my true love gave to me: "; | ||
| for (int j = i; j >= 1; --j) { | ||
| if (j == i) { | ||
| verse += std::string{gifts[j]}; | ||
| } else if (j == 1) { | ||
| verse += std::string{", and "} + std::string{gifts[j]}; | ||
| } else { | ||
| verse += std::string{", "} + std::string{gifts[j]}; | ||
| } | ||
| } | ||
| verse += ".\n"; | ||
| result += verse; | ||
| } | ||
| return result; |
There was a problem hiding this comment.
There's quite a bit of redundancy in here. You had that better in a previous iteration. But don't worry, I will take care of it.
Uh oh!
There was an error while loading. Please reload this page.
alienx5499
commented
Aug 30, 2025
@ahans Thank you for your time, review, and for merging! 🤍 |
Description
This PR adds the missing twelve-days practice exercise to the C++ track, addressing issue #985.
What this PR does
twelve-dayspractice exercise implementation.meta/directoryconfig.jsonwith exercise metadataExercise Details
Exercise: Twelve Days of Christmas song lyrics generator
Difficulty: 3 (Medium)
Author: alienx5499
Functions Implemented:
verse(int day)- Returns lyrics for a specific daylyrics(int start_day, int end_day)- Returns multiple verseslyrics(int end_day)- Returns verses 1 through end_dayFiles Added:
Testing
./bin/configlet sync -e twelve-days(up-to-date)Implementation Notes
Related Issue
Addresses #985 (adds one of the missing practice exercises)
Ready for review! This exercise follows all Exercism C++ track conventions and is fully functional.