Code review comment for lp:~allenap/launchpad/lp-app-longpoll

Revision history for this message
Graham Binns (gmb) wrote :

Hi Gavin,

Nice to see all the work from last week landing. This is a good branch; a few nits but nothing huge. r=me pending the changes below.

[1]

396 +class TestModule(TestCase):

This seems like a bit of a generic name... Maybe TestSubscriberModule for clarity?

[2]

446 + source = Attribute(
447 + "The event source.")
448 +
449 + event = Attribute(
450 + "An object indicating the type of event.")

Minor stylistic nit: I don't think these need to be spread over two lines.

[3]

556 +class TestModule(TestCase):

Same as above. TestLongPollModule maybe?

[4]

560 + def test_subscribe(self):

576 + def test_emit(self):

681 + def test_register_and_unregister(self):

DMMT. These could do with some leading comments explaining the expected behaviour so that I don't have to read the tests.

review: Approve (code)

« Back to merge proposal