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

Add bottle-song #2384

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Add bottle-song #2384

wants to merge 2 commits into from

Conversation

BNAndras
Copy link
Member

See https://forum.exercism.org/t/add-bottle-song-exercise/15761/2.

This PR also deprecates Beer Song in favor of Bottle Song. The practice exercise generator script gave me blank files, but I was able to cobble together what I needed.

@ErikSchierboom
Copy link
Member

@BNAndras Could you try using: pwsh bin/add-practice-exercise.ps1 -d 3 -a erikschierboom bottle-song. That should give you a generator file. Note: before running the above script, please pull in the latest main first.

@BNAndras
Copy link
Member Author

I pulled in the latest main, but that command didn't give me any different output from before. I see the empty files for the test suite, stub file, and example solution that configlet would give me normally, a filled-out csproj, a filled-out .editorconfig, and an updated Exercises.sln.

@ErikSchierboom
Copy link
Member

Ah, I might have figured out the issue. I'll work on a fix.

@ErikSchierboom
Copy link
Member

@BNAndras I'm hoping #2386 fixes it. Please rebase on the latest main

@BNAndras
Copy link
Member Author

Yeah, I see generated tests now. 🎉

[Fact{{ if !for.first }}(Skip = "Remove this Skip property to run this test"){{ end }}]
public void {{ test.testMethod }}()
{
Assert.Equal({{ test.expected }}, {{ testedClass }}.{{ test.testedMethod }}({{ test.input.startBottles }}, {{ test.input.takeDown }}));
Copy link
Member

Choose a reason for hiding this comment

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

I think it would help with readability to put the expected value in a variable:

Suggested change
Assert.Equal({{ test.expected }}, {{ testedClass }}.{{ test.testedMethod }}({{ test.input.startBottles }}, {{ test.input.takeDown }}));
string[] expected = [
{{- for line in test.expected }}
{{ line | string.literal }}{{- if !for.last }},{{ end -}}
{{ end }}
];
Assert.Equal(expected, {{ testedClass }}.{{ test.testedMethod }}({{ test.input.startBottles }}, {{ test.input.takeDown }}));

Copy link
Member Author

Choose a reason for hiding this comment

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

I concur. Ideally, the expected value should span multiple lines for readability, but I’m not sure how that generator could handle that.

Copy link
Member Author

Choose a reason for hiding this comment

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

On second look, I think that's what the template is doing actually. I assumed ./bin-update-tests.ps1 bottle-song would regenerate the test suite using the updated generator template, but nothing happened for me after running that command.

@ErikSchierboom
Copy link
Member

@BNAndras Looks good. I'm working on fixing the test runner properly, so I'll need to hold off on merging this until that is fixed.

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.

2 participants