fix(anagram): allow arbitrary order of words in result

[ahans]

Here's my proposal on how we could accept any order of words for the anagram exercise.

[github-actions]

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!

[vaeng]

I like not having order in the returned data structure, to keep it simple and close to the intended exercise text.

I would have used a set as well. I wondered if we should make it possible for the students to use any sensible std container?

[ahans]

I like not having order in the returned data structure, to keep it simple and close to the intended exercise text.

@IsaacG in the other PR says the same, so let's go with this PR and try to make it as good as possible.

I would have used a set as well. I wondered if we should make it possible for the students to use any sensible std container?

Yes, I'm working on that now. It should be possible, but it will not make the test code more readable. I really wonder if that is worth it for an exercise as easy as anagram. Anyway, let's continue the discussion when I have something ready.

[ahans]

Btw, since version 3.0.6 Catch2 has matchers integrated that would make this probably very easy. With the Catch2 files copied all over the repo, upgrading just for this matches is probably not reasonable. But another case showing that we should really push for improving that.

[vaeng]

The Test-Runner already has some fixes in place to run old catch versions, but the concept exercises run with the new catch version to support tags. We can thus use any catch version we please on a per-exercise base

[ahans]

The Test-Runner already has some fixes in place to run old catch versions, but the concept exercises run with the new catch version to support tags. We can thus use any catch version we please on a per-exercise base

Can you give some pointers how this is implemented? The copies of catch.hpp that I checked in the repo also under concept are also v2.13.6, which is also true for anagram. Also when I download a concept exercise such as lasagna I get the same catch.hpp. Either way, I'm not sure it's a good idea to have different Catch versions in the repo, even if it was supported by the tooling.

[vaeng]

I think this makes it nice and easy for the students.

The only thing I would add is a small comment to explain the need for the class and that students can solve it with any container they like. Do you think that would help shipping the intent of the changes?

[ahans]

I coded something up now that supports any container that supports std::size(), std::cbegin(), and std::cend(). I could have just templated the to_set I had before, but I wanted something that is closer to the Catch2 matchers. Ideally, I would have been able to implement a matcher that would be used like this:

REQUIRE_THAT(matches, IsIdenticalTo({"gallery", "regally", "largely"}));

However, with the old version of catch, which only supports old-style matches, that is not possible. The reason being that they use some sort of CRTP where you have to pass the type that match() will be called with to the parent class. Without some decltype(matcher), I didn't see a way how to make that happen, and having that in each test (or deferring to helper) I wouldn't like. So what we have now is the compromise.

[ahans]

I think this makes it nice and easy for the students.

The only thing I would add is a small comment to explain the need for the class and that students can solve it with any container they like. Do you think that would help shipping the intent of the changes?

Yes, for sure! I was also thinking about doing a forward declaration and putting the implementation at the bottom of the file. Do you think that would help reduce confusion or only add more?

[ahans]

I now also added the comment. Let me know if this looks good to you and also what you think about having a forward declaration and moving the actual implementation of the class to the bottom of tile file.

[vaeng]

Forward declaration is a great idea!

[ahans]

Forward declaration is a great idea!

It doesn't work with a classic forward declaration, unfortunately. The only thing I could do is the standard declaration/definition split, but then we'd still have a lot of code at the top. I'd rather keep it as is. What do you think?

[ahans]

I pushed some more minor improvements:

• Removed anonymous namespace; while I'd still consider it a best practice, I think it confuses students more than the code benefits from this.
• Removed default constructor so that we need to pass an explicit empty initializer_list; I think this is clearer and more consistent.
• Changed to old-school parenthesis-init for ExpectedSet instead of curly braces. The curly braces I still consider best practice in modern C++, but here I see the chance of more confusion with the double braces in {{. Having ({ ... }) could make things easier to understand.
• Improved wording of comment for ExpectedSet.

@vaeng If you think this is good now, I would squash and merge. You have approved already, but quite a lot has changed since then, so that I'd appreciate it if you could have another look. Thanks!

[vaeng]

All good changes, which should help students to pass this exercise and read the instructions in a clear way. Thanks for the effort.