Skip to content

beer-song: replace with bottle-song #2021

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

Merged
merged 7 commits into from
Feb 20, 2025

Conversation

ellnix
Copy link
Contributor

@ellnix ellnix commented Feb 14, 2025

No description provided.

@senekor
Copy link
Contributor

senekor commented Feb 14, 2025

Let's do the deprecation of the old exercise together with the implementation of the new one.

@ellnix ellnix changed the title beer-song: deprecate beer-song: replace with bottle-song Feb 14, 2025
@ellnix ellnix marked this pull request as draft February 14, 2025 20:47
@ellnix ellnix force-pushed the beer-song-deprecation branch from ce621ba to fc4dac2 Compare February 19, 2025 21:45
@@ -9,7 +9,12 @@ use bottle_song::*;
fn {{ test.description | make_ident }}() {
assert_eq!(
recite({{ test.input.startBottles }}, {{ test.input.takeDown }}),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw that beer-song called trim() on this value, but that just feels like cheating...

It's up to the student to use trim() if they need it, in my opinion.

@ellnix
Copy link
Contributor Author

ellnix commented Feb 19, 2025

Thought concat! would make the tests easier to read, if you disagree I can revert the commit easily.

@ellnix ellnix requested a review from senekor February 19, 2025 21:50
@ellnix ellnix marked this pull request as ready for review February 19, 2025 21:50
recite({{ test.input.startBottles }}, {{ test.input.takeDown }}),
concat!(
{% for line in test.expected %}
"{{line}}{% if not loop.last %}\n{% endif %}",
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: keep the trailing newline

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went through other repos after receiving this feedback, I had only looked at Elixir which does not have the newline. Ruby seems to have the newline, and python, javascript, and go seem to return some sort of array of strings.

Should we just go for a Vec of Strings?

I'll keep the trailing new line in the meantime, even though I think that the challenge makes more sense without it. But it's no big deal.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we just go for a Vec of Strings?

Nah, that seems weird to me.

In my mind, text that consists of multiple lines should always have a trailing newline. But it's not important. Neither alternatives result in a more or less interesting / challenging exercise.

Maybe that's what the trim call was for. Allow students to implement their preference, with or without trailing newline.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can add trim if you want, or else I guess we can merge this as is?

Copy link
Contributor

Choose a reason for hiding this comment

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

yeah whatever you want 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added trim(), feel free to merge if you have no reservations.

@senekor senekor merged commit 16a072e into exercism:main Feb 20, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants