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

Allergies' "test_allergic_to_everything" implicitly requires a specific list ordering #186

Closed
spthm opened this issue Jul 2, 2015 · 3 comments

Comments

@spthm
Copy link

spthm commented Jul 2, 2015

def test_allergic_to_everything(self):
    self.assertEqual(
        ('eggs peanuts shellfish strawberries tomatoes '
         'chocolate pollen cats').split(),
        Allergies(255).list)

This requires that Allergies(255).list return items in exactly the same order as they are written above. If the order is not important, assertItemsEqual() is better. Unfortunately, in Python 3 this was renamed to assertCountEqual(), which does not exist in Python 2.7. So perhaps assertEqual(sorted(expected), sorted(actual)) would in fact be the best solution here.

Alternatively, if the requirement for identical ordering is intentional, it should be made clearer using assertSequenceEqual(). At present, the README merely states that it should return "All the allergens Tom is allergic to."

I believe this may also affect other Python exercises.

I am happy to submit a pull request for said changes, but would first like to know if there is any consensus on exactly what the correct behaviour should be: to require identical ordering or not?

@Dog
Copy link
Contributor

Dog commented Jul 2, 2015

Order isn't important in the result, though the order does in a way give a hint it is related to binary. Feel free to do a pull request, because I think it would be better to not force order.

We can handle the change between Python 2 and 3 without too much of an issue. Using a library like six would make it easier, but it would probably be difficult to make people install it.

I think we would could do something along the lines of:

if sys.version_info < (3,):
      assertItemsEqual()
else:
      assertCountEqual()

for the time being.

@Dog Dog mentioned this issue Jul 3, 2015
@ZacharyRSmith
Copy link
Contributor

I assertEqual(sorted(expected), sorted(actual)) in commit 7ba9220 of pull request #194

@kytrinyx
Copy link
Member

That works for me, thanks!

This issue was closed.
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

No branches or pull requests

4 participants