[Mecha Munch Management] Add test case for send_to_store of extra item on isle_mapping

Programming Python
1

I’ve mentored a few students on their Mecha Munch Management solutions, and some of them had implemented task#5 (Send User Shopping Cart to Store for Fulfillment) in a way that have a bug but pass the tests.

On send_to_store function we are told: “The function should return a combined “fulfillment cart” that has (quantity, aisle, and refrigeration) for each item the customer is ordering”. Function’s arguments are: customer’s cart & store’s aisle_mapping.

Some students solutions looped over aisle_mapping and not over cart. If there is an item in aisle_mapping thats not in cart , we’ll get a KeyError exception or item be in the returned fulfillment dictionary. In the real app we’ll expect store’s aisle_mapping to have information about all of store’s items, so this is a likely scenario.

I suggest adding the case to check the above. Here is a commit on forked repository to be used for PR if you approve.

This was an initiative of @snovalnikov, the last student I’ve mentored on this exercise. He wanted to open this discussion, but probably too busy at this time.

2

I appreciate you bringing up this question, @izmirli . I believe implementing a systemic solution would be an effective way to address this issue.

3

Hi @BethanyG, can you take a look at this when you’ll have time? Thanks!

4

Hi @izmirli - could you please give me some examples of student code that is passing tests but flowed in your opinion? Thanks!

5

Here - student#1:

def send_to_store(cart, isle_mapping):
    """Combine users order to isle and refrigeration information.
 
    :param cart: dict - users shopping cart dictionary.
    :param isle_mapping: dict - isle and refrigeration information dictionary.
    :return: dict - fulfillment dictionary ready to send to store.
    """
    for key, item in isle_mapping.items():
        cart[key] = [cart[key]] + item
    return dict(sorted(cart.items(), reverse=True))

student#2:

def send_to_store(cart, isle_mapping):
    unsorted_result = {idea: [cart[idea]] + isle for idea, isle in isle_mapping.items()}
    return dict(sorted(unsorted_result.items(), reverse=True))

If isle_mapping have more items than cart’s, we’ll get a KeyError exception.

For reference, here is meta/exemplar.py solution:

def send_to_store(cart, aisle_mapping):
    fulfillment_cart = {}

    for key in cart.keys():
        fulfillment_cart[key] = [cart[key]] + aisle_mapping[key]

    return dict(sorted(fulfillment_cart.items(), reverse=True))
6

Hi @izmirli :wave:

Apologies for the long delay in getting back to you. Yes! – please go ahead and make a PR to the content repo for these test changes. :smile: – and thank you for proposing them. Two favors to ask as you do that:

  1. Maybe add one more additional test case so that students clearly get the picture. One case might look like a “mistake”, but failing two will strongly nudge the student to (maybe?) check their methodology.

  2. We recently updated the spelling of aisle (vs isle - which is an error :woman_facepalming: ) for this exercise, please make sure that those changes are picked up in any fork/branch you’re PR-ing from. :smile:

The PR will close – but I will reopen and review/merge.

7

Here is the [now closed] PR:

  1. I’ve tried to follow your requests., hopefully all in successful manner. Please tell me if any changes are needed.
  2. It seem there where redundant empty line (# 34) and space (# 133), so I’ve removed them as well.
  3. Indentation of closing brackets were changed automatically by my editor - hope this is acceptable. Shouldn’t a formatter, like Black, be applied as part of CI?
8

Here is the [now closed] PR

This has been tested, reviewed and merged. :smile:

It’s not, really – for some reason, I didn’t notice it when reviewing, but went back and fixed it just now.

We use Black for our practice exercise formatting, but I have found Black quite problematic for concept exercises.

This is partly due to us formatting the test files in a particular way to be more readable/maintainable - and partly due to Black’s unfortunate handling of nested data structures (it makes an utter mess of them, the more nested, the bigger the mess).

While we can (now that they’ve finally introduced a skip) insert skips, I’d rather not have to clutter the files with skip notes or worry about having to reformat something like this or this when Black mucks it up.

This might change in the future, but because we’d need to test things out, it’s not likely to change any time soon.