Big jump from finger18.py to finger19.py in tutorial.

[fresh]

There's a big change in code here to use component-type stuff witghout much
explanation.
Need to really explain that calling an interface looks up an adapter, etc. Might
be best to say "read the components how-to" with link ;-)
Would also be good to explain that protocol.ClientFactoru uses the protocol
instance variable by default, AND that it is that that implements the
buidlProtocol method required for most of the CientFactory interfaces in the
example.

[hypatia]

Thanks, just noting the bug is read. More comments (or fixes) when I look at the
tutorial.

[hypatia]

As dialtone pointed out in issue 547, one specific example is that finger19.py
defines IFingerSetterService without using it (it is used in finger19a.py).
I'm planning to split the tutorial into about four parts (because it's such an
enormous download) before I fix this bug, I'll keep you posted on progress.

[hypatia]

Reassigning doc bugs to edsuom.

[thijs]

(In [24841]) Branching to 'finger19-cleanup-532'

[thijs]

Replying to hypatia:

As dialtone pointed out in issue 547, one specific example is that finger19.py defines IFingerSetterService without using it (it is used in finger19a.py).

I removed IFingerSetterService in r24842.

I'm planning to split the tutorial into about four parts (because it's such an enormous download) before I fix this bug, I'll keep you posted on progress.

I assume this not happening anymore, putting this up for review so it can finally be closed, unless there are other things that can be cleaned up here.

[exarkun]

Hm. I think there's still a big jump here, although now it's slightly smaller. :)

However, it seems like almost every change in the finger tutorial is unexplained. components.xhtml seems to actually have more explanatory text than other parts of the tutorial.

There's probably a huge amount of improvement that could be made to the finger tutorial overall. We can do it incrementally, perhaps, but just shortening finger19.tac (or even doing that and adding more explanatory text to components.xhtml) isn't going to do it.

As far as this particular part of the document goes, it would probably benefit greatly from some links to zope.interface documentation, documentation about the Twisted adapter registry (do we have any? I don't know), and the removal the lies (or at least unexplained claims) like However, using interfaces provides the same flexibility inheritance gives: future subclasses can override the adapters..

Removing IFingerSetterService but leaving FingerSetterFactoryFromService doesn't really make sense. As far as I can tell, FingerSetterFactoryFromService isn't used anywhere (but this code is ridiculously complex). If the interface is removed, then removing this service makes sense too.

At the very least, components.registerAdapter requires 3 arguments, so the change beneath FingerSetterFactoryFromService to call it with only two is wrong.

Also, the later listings still include IFingerSetterService, so deleting it from finger19.tac only moves the jump from 18->19 to 19->20. :)

[kehander1]

"Component-based architecture" with much more explanation

[kehander1]

There we go. It's still not pretty, but it's an improvement.

[exarkun]

1. The document still contains lies (such as the one I pointed out in my last review).
2. I think the explanation of how an interface is defined in terms of its lexical structure (eg, each interface is defined with a line like) is unnecessarily reductionist. Assume the reader knows what a class is, assume they know what the word "subclass" means, and so on.
3. This sentence If <code>IFingerFactory</code> was instantiated here with an object that actually implemented <code>IFingerFactory</code>, we would get that object. seems to contain a thinko - IFingerFactory shouldn't appear there twice, should it?
4. Python doesn't have commands - the registerAdapter function call shouldn't be described as one.
5. The case referenced by in this case, <code>FingerService</code> actually involves an instance of FingerService, not FingerService itself.
6. Hm. The new part of the document which gives a list of other adapters makes it really clear that there's too much code in finger19.tac.
   1. IFingerFactory serves no purpose here. It defines two methods, only one of which is actually called on the object which results from adapting an object to the interface, and two other methods which are called on that object aren't part of the interface. The interface and the code using it should be deleted. FingerFactoryFromService should be instantiated directly. Unless there is more than one possible implementation of an interface (either the interface adapted from or the interface adapted to), callable-interface-object adaption adds no value.
   2. Ugg. I was going to list some more things, but it seems all uses of adaption in this example and document are wrong. It's possible fixing all of this is beyond the scope of this ticket. On the other hand, the "big jump" here seems to be from a version of the code that is perhaps not ideally factored but which is simple and makes sense, to the bottom of a cliff. Not jumping off the cliff might be a good idea. IFingerService and IFingerSetterService could make sense. IFingerFactory doesn't really: it should have the other server factory methods added to it, or it should be deleted and adaption shouldn't be used for that part of the code. Likewise for IFingerSetterFactory and for IIRCClientFactory (IIRCClientFactory particularly doesn't make sense - if the point here is to abstract different possible implementations away from the finger code, then stuffing IRC right into the name of one of the interfaces makes no sense to me).

I'm not sure it makes sense to give any more feedback beyond those last points, since a lot about what happens to this document depends on how those points get addressed. If you'd like clarification on any of these points, or if you'd like to discuss a plan for this document overall, I'd be happy to talk about this some more (either here or on IRC).

[kehander1]

Okay then. "Ugg" is indeed the word, but I think this looks a lot better. I have linked to the Components HOWTO rather than trying to cram a bad explanation in.

Changes to finger19.tac (and derivatives) include the removal of BuildProtocol? from the interface definitions. I also felt like renaming UserStatusTree? to something that provides more parallelism. And of course the FingerSetterFactory? nastiness is rearranged. (I also notice that, unlike finger18.tac, IRCClientFactory no longer does anything with ReconnectingClientFactory?, but I'm not sure where that should go.)

The phrase However, using interfaces provides the same flexibility inheritance gives: future subclasses can override the adapters makes sense to me, albeit not perfect sense.

[kehander1]

"Component-based architecture" with much more explanation, and new finger19x.tac

[thijs, exarkun]

(In [25110]) Branching to 'finger-components-jump-532'

[exarkun]

(In [25111]) apply components-v2.diff

refs #532

[thijs]

Testing trac ticket email notification.

[thijs]

Removing review keyword left in by kehander1

[exarkun]

It's still up for review. I didn't review it.

[glyph]

1. The whole paragraph that begins with "Zope is described as" is terrible.
   1. The point being made there is that we don't actually care about zope at all, so it definitely should not be the first sentence of the first paragraph of a section. Instead it should start off describing what is relevant.
   2. The package in question is zope.interface, not zope.interfaces.
2. The paragraph that's parenthetical probably shouldn't be.
3. Style dictates that docstrings should be formatted

       """
       Like this.
       """
   

   If you are trying to compress them to save vertical space, it would be better to compress them

       "Like this."
   

   rather than putting them on a single line but using triple-quotes anyway.
4. The parenthetical "We are of course" would be better expressed non-parenthetically, omitting the 'of course', and with a hyperlink to the appropriate portion of the coding standard. (You would have to add the appropriate portion to the coding standard, since apparently it doesn't actually say that!)
5. In general I think it's best to put code (like the IFingerService, IFingerService, and FingerFactoryFromService declarations) that is a full, correct Python file into a separate listing file, rather than using an inline <pre class="python">. Eventually this might help to reduce the amount of code in the various .tac files (although I'm not saying that needs to happen in this branch).
6. "we'll have to register it in the main function" - this is both abrupt and inaccurate. The registration does not take place inside a function, let alone a "main" function. It would also be good to refer to some documentation that explains what registerAdapter does.

[kehander1]

Changes to branch 25111

[kehander1]

1. I think it's important to have something about what Zope is, lest the unfamiliar user feel it necessary to head over to zope.org and confront the bewildering variety of documentation there.
2. (Fixed.)
3. Listings throughout the documentation (not just the finger tutorial) use one-line docstrings with that kind of formatting. PEP 257 also agrees. (Actually, PEP 257 says a lot of things...)
4. It seems like something a little too readily apparent for the coding standard.
5. In order to make each of those full, correct TAC files, they'd all have to contain identical, additional lines of code that might make it less clear as to what exactly is changing.
6. As mentioned previously, there doesn't seem to be any documentation as to what registerAdapter does, and I'm not sure it's strictly necessary so long as it's clear as to how it is used. I'm also not sure what's better than "in the main function", so now it's "outside the specification".