Ch3 Comments

[xtaje]

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L192
For the id generation, fluent helpers will make the code flow read a bit easier.

def random_sku(id_=None):
    return random_ref(''.join(['s', str(id_)]))

def random_order(id_=None):
    return random_ref(''.join(['o', str(id_)]))

def random_batch(id_=None):
    return random_ref(''.join(['b', str(id_)]))

@pytest.mark.usefixtures('restart_api')
def test_something():
    sku, order = random_sku(), random_order()
    sku1, order1 = random_sku(1), random_order(1)
    # etc., etc.

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L320
FWIW, pytest.raises also works with a regex

with pytest.raises(InvalidSkuException, match="nonexistentsku"):
    doit()

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L339
Is there supposed to be an @DataClass annotation on FakeSession? Otherwise there is a class instance and object instance "committed" variable. If not, deleting the "committed = False" line will be OK.

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L347
Some tangential commentary, on other styles/options here. The shunt/self-shunt pattern is also handy for this scenario, where you just need some provisional implementation. (Example later.) and the "FakeSession" in most of the examples is a dummy, rather than a fake, and a spy in two others. This could be a sidebar into discussing test doubles, mocks vs stubs, and/or London/Detroit testing. For the dummies, this would work:

# MagicMock is just creating a dummy used to satisfy the API.
# We never make any assertions on it.
services.allocate(line, repo, MagicMock())

For the spies a shunt could use just use a closure (although this makes the examples more verbose):

def test_commits():
    line = model.OrderLine('o1', 'sku1', 10)
    batch = model.Batch('b1', 'sku1', 100, eta=None)
    repo = FakeRepository([batch])

    commited = False
    class FakeSession():
        def commit(self):
            committed = True

    services.allocate(line, repo, FakeSession())
    assert session.committed is True

Or the third option could be a proper mock, with an assertion along the lines of session.commit.assert_called_once.

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L589
Are the source code comments supposed to be "domain-layer" and "service-layer" instead of
"model-layer" and "domain-layer"? Otherwise I'm a bit confused.

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L944
/add_batch route looks a bit funny to me because it's not quite RESTful, and more like RPC over HTTP. Something like POST /batches would make for a much more obvious boundary between "outside" and "inside", or "addapter" and "domain" model.

That could then help drive discussion around "Clean Architecture". The REST API does not involve Inversion-Of-Control or Dependency Inversion, and would be handy for comparing/contrasting the ideas behind "Ports-And-Adapters" vs "Functional-Core/Imperative Shell."

---

https://github.com/python-leap/book/blame/master/chapter_03_service_layer.asciidoc#L1081
I think the diagram makes sense, but would have more impact if there were three
diagrams instead of two -- one was for the abstractions, a second for the tests, and a third for the application.

They would all have identical topologies and layouts, but different labels (e.g. FakeRepository, AbstractRepository, SqlAlchemyRepository). That diagram could then be used to re-visit the idea of roles and collaborations.

Also, the "layers" at this point feel to me like they start to make less sense. Maybe just having a graph of depdendencies will be better (like in the Seeman blog post.)

---

For the Flask APIs, the blocks seem like they could flow a little easier with some small changes, and are missing a tiny bit of extra error handling.

If you have a db connection error it will tank the route handler and probably return a very unfriendly Flask-default response. (This might also be another point to talk about FunctionalCore/ImperativeShell.)

For example, for allocate route, my preference would be to do something like:

@app.route("/allocate", methods=['POST'])
def allocate_endpoint():
    try:
        session = get_session() 
        repo = repository.SqlAlchemyRepository(session)
        line = model.OrderLine(
            request.json['orderid'], 
            request.json['sku'],
            request.json['qty'], 
        )
        batchid = services.allocate(line, repo, session) 
        return jsonify({'batchid': batchid}), 201 
    except (model.OutOfStock, services.InvalidSku) as e:
        return jsonify({'message': str(e)}), 400
    except WhateverExceptionGetsThrownForDbConnectionFailures as e:
        return jsonify({'message': 'Oops! We'll be back'}), 503

[hjwp]

Is there supposed to be an @DataClass annotation on FakeSession? Otherwise there is a class instance and object instance "committed" variable. If not, deleting the "committed = False" line will be OK.

that was me. intention is to have the default .committed be False for all instances, but any instance that calls commit() will get its attribute set to True.

this may be a bit of Python weirdness?

>>> class Classy:
...     attribute = False
... 
>>> instance = Classy()
>>> instance.attribute
False
>>> instance.attribute = True
>>> instance.attribute
True
>>> Classy.attribute
False

[xtaje]

Re: the fake session, I see what you were going for (kind of a "Copy-on-Write"). It is a bit unusual, and looks maybe like it was a mistake The scenarios I am thinking are things like this:

class Thing(object):
    stack = [1, 2, 3]

    def pop(self):
        return self.stack.pop()

    def push(self, val):
        return self.stack.append(val)

def test_first():
    assert Thing().pop() == 3

def test_second():
    assert Thing().pop() == 3

def test_third():
    thing = Thing()
    thing.push(100)
    assert thing.stack == [1, 2, 3, 100]

If you run that with a test randomizer or a parallel-runner using threads, the tests looks will look like they fail randomly due to the shared state.

[hjwp]

I suppose people are going to cargo-cult copy these sorts of code listings so maybe we should be careful not to leave footguns like that lying around? will add a note to change it when i get a moment... thanks Ed.

[hjwp]

re: the first comment about making the random-reference-generators more readable, i found that quite appealing. here's what i've been playing around with so far:

def random_suffix():
    return uuid.uuid4().hex[:6]

def random_sku(name=''):
    return f'sku-{name}-{random_suffix()}'

def random_batchref(name=''):
    return f'batch-{name}-{random_suffix()}'

def random_orderid(name=''):
    return f'order-{name}-{random_suffix()}


@pytest.mark.usefixtures('postgres_db')
@pytest.mark.usefixtures('restart_api')
def test_happy_path_returns_201_and_allocated_batch():
    sku, othersku = random_sku('desirable-chair'), random_sku('ugly-chair')
    oldbatch, newbatch = random_batchref('old'), random_batchref('new')
    otherbatch = random_batchref('other')
    post_to_add_batch(oldbatch, sku, 100, '2011-01-02')
    post_to_add_batch(newbatch, sku, 100, '2011-01-01')
    post_to_add_batch(otherbatch, othersku, 100, None)

    data = {'orderid': random_orderid(), 'sku': sku, 'qty': 3}
    url = config.get_api_url()
    r = requests.post(f'{url}/allocate', json=data)
    assert r.status_code == 201

    assert r.json()['batchref'] == newbatch

worth it? overengineered? what do y'all think?