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.

Sign up for GitHub

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

Jump to bottom

Change anagram test case input #2314

Merged
merged 1 commit into from
Sep 22, 2023
Merged

Change anagram test case input #2314

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions exercises/anagram/canonical-data.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,6 +132,17 @@
},
"expected": []
},
{
"uuid": "630abb71-a94e-4715-8395-179ec1df9f91",
"reimplements": "7cc195ad-e3c7-44ee-9fd2-d3c344806a2c",
"description": "does not detect an anagram if the original word is repeated",
"property": "findAnagrams",
"input": {
"subject": "go",
"candidates": ["goGoGO"]
},
"expected": []
},
{
"uuid": "9878a1c9-d6ea-4235-ae51-3ea2befd6842",
"description": "anagrams must use all letters exactly once",
Copy link
Member

Choose a reason for hiding this comment

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

Should we state "all letters of the source word" or something similar?

Copy link
Member

Choose a reason for hiding this comment

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

Isn't that a different test case? I mean, I don't disagree, but it looks like a different test case?

Copy link
Member

Choose a reason for hiding this comment

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

Does that mean that the description, then, is not right in general?

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 so

Copy link
Contributor Author

@olapokon olapokon Sep 21, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

@kotp Did you mean change "anagrams must use all letters exactly once" to "anagrams must use all letters of the source word exactly once"? As @ErikSchierboom says this is the testcase below. Or did you mean change "does not detect an anagram if the original word is repeated" to "does not detect an anagram if all letters of the source word are repeated"?

If you meant the former, is the description ok as it is now? It currently is "does not detect an anagram if the original word is repeated", which seems to fit the test case even after removing the spaces, since it is the original word being repeated, just without spaces in-between.

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 your change is fine on lines 135-145, so we are really talking about what you have not changed, and may change, as suggested.

Additionally, if you choose not to fix it while we are here, that is OK too. It may be changed at another time, in another pull request.

Copy link
Member

Choose a reason for hiding this comment

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

Well, I must say that your change may be redundant if the following test case is described better. It is not described well at the moment. It may be enforcing the same thing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It depends on how one looks at it I guess. The original reason for this change was that the existing test is problematic due to the whitespace. I can change the next test's description as suggested if you prefer, or close the pull request and leave the goGoGO test as it is until it can be deprecated/removed completely.

Copy link
Member

Choose a reason for hiding this comment

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

Let's merge this as is and do the update of the other test case in a separate PR.

Expand Down