Skip to content

Conversation

meatball133
Copy link
Member

Includes:

  • Circular Buffer
  • Crypto square
  • Custom set
  • Darts
  • Diamond
  • Differnce of square
  • Dnd character

Copy link
Contributor

This PR touches files which potentially affect the outcome of the tests of an exercise. This will cause all students' solutions to affected exercises to be re-tested.

If this PR does not affect the result of the test (or, for example, adds an edge case that is not worth rerunning all tests for), please add the following to the merge-commit message which will stops student's tests from re-running. Please copy-paste to avoid typos.

[no important files changed]

For more information, refer to the documentation. If you are unsure whether to add the message or not, please ping @exercism/maintainers-admin in a comment. Thank you!

* Enhance underscore method to filter invalid characters and update test file naming convention
* Add test case which test underscore for special characters
* Use character classes in regex. Also expose description for nested test cases.
* Underscore must not create multiple underscores around special characters
Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Non-blocking suggestion.

@@ -47,7 +47,7 @@ def additional_json(json)
def remove_tests(uuid, json)
json["cases"].each_with_object([]) do |x, acc|
if x["cases"]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if x["cases"]
acc << if x["cases"]

This, along with ...

Copy link
Member Author

Choose a reason for hiding this comment

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

This seems to create error when I do it that way

Copy link
Member

Choose a reason for hiding this comment

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

Can you show the patch, how you attempted this?

Copy link
Member Author

Choose a reason for hiding this comment

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

1698906. And you see that when main was merged in it failed. But when rollbacked it worked: 1b71ff2

Comment on lines 50 to 52
acc << { "cases" => remove_tests(uuid, x), "description" => x["description"] }
elsif uuid.include?(x["uuid"])
acc << x
Copy link
Member

Choose a reason for hiding this comment

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

... with this...

Suggested change
acc << { "cases" => remove_tests(uuid, x), "description" => x["description"] }
elsif uuid.include?(x["uuid"])
acc << x
{ "cases" => remove_tests(uuid, x), "description" => x["description"] }
elsif uuid.include?(x["uuid"])
x

... removes the unconditional work from the conditional branches.

I did not touch the double quotes, though. Unsure if that is already attended to in another place, if we are doing that.

def test_largest_possible_diamond
skip
answer = Diamond.make_diamond('Z')
string = " A \n"\
Copy link
Member

Choose a reason for hiding this comment

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

Consider using HereDoc for this, rather than consecutive string "trick".

Copy link
Member

Choose a reason for hiding this comment

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

Same for the others.

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 tested it out but the problem I find is that for this exercise is trailing spaces important. So what end up happening is that each line is appended with #{' '}. Which in my opinion, looks just weird.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with that, many people do not notice the white space that is not supposed to be there, they are surely going to miss the whitespace that is supposed to be there.

I have done it with a HereDoc and dynamic format codes, and have done it with append and multiplied spaces, so there are a few ways to address that... but for the tests, it needs to be clear to the reader.

Copy link
Member

@kotp kotp left a comment

Choose a reason for hiding this comment

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

Trailing whitespace.

class CircularBufferTest < Minitest::Test
<% json["cases"].each do |cases| %>
def test_<%= underscore(cases["description"]) %>
<%= skip? %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<%= skip? %>
<%= skip? %>

class DartsTest < Minitest::Test
<% json["cases"].each do |cases| %>
def test_<%= underscore(cases["description"]) %>
<%= skip? %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<%= skip? %>
<%= skip? %>

<% json["cases"].each do |cases| %>
<% cases["cases"].each do |sub_case| %>
def test_<%= underscore(sub_case["description"]) %>
<%= skip? %>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<%= skip? %>
<%= skip? %>

@kotp kotp merged commit 05d72ff into main Sep 7, 2025
5 checks passed
@kotp kotp deleted the add-template-batch3 branch September 7, 2025 17:13
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