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".

[exarkun]

(In [25499]) Apply components-25111.diff

refs #532

[exarkun]

1. There's lots of trailing whitespace in the additions to components.xhtml.
2. The last two sentences of the paragraph before the paragraph about zope.interface still don't add anything to the document. I still think they detract, actually. The last sentence is at best incoherent, at worst simply false. I don't know how subclasses would get involved (what are they subclasses of?), but you cannot override an adapter registered with registerAdapter. Period.
3. The new language about Zope is better, but I agree with Glyph that it's misplaced. It's important to emphasize that Twisted does not depend on Zope, but I don't think this documentation benefits from including an explanation of what Zope is, however brief.
4. The introductory text about implements still talks about inserting lines places. This would be better if it talked about calling functions.
5. Nothing has been done about the fact that the use of interfaces and adapters in this example is pointless. IFingerFactory, FingerFactoryFromService, IIRCClientFactory, IRCClientFactoryFromService. Separating unrelated concerns into different classes is great, but you don't need an adapter registry to do it. FingerFactoryFromService can do its job just as well without IFingerFactory and without being registered as an adapter. Likewise for the other classes filling similar roles. I can only see someone reading this document and getting some bad ideas about how to structure their code. We should not encourage people to add this kind of pointless complexity to their projects.
6. Another significant problem is that the tacs which follow finger19.tac still include all of the old definitions. Whatever changes are made to finger19.tac need to be reflected in all the later code listings and any prose which talks about them. Removing the duplication by factoring re-used parts of the tutorial listings into modules so that the definitions can be shared seems like it would be a good idea.

To re-iterate what is the most important part of this (and previous) review, though: the use of adapters and interfaces in finger19.tac is incorrect. No amount of clear explanation or code re-arrangement is going to fix this.

I really don't want all the effort you've put into this ticket to turn out to be fruitless, but I also don't want you to spend any more time on it if that will be fruitless. I suggest that if you're interested in doing more work here, you work on removing the use of interfaces and the adapter registry. For example, this approach:

ircFactory = IRCFingerFactory(service, 'fingerbot')
ircClient = internet.TCPClient('irc.freenode.org', 6667, ircFactory)
ircClient.setServiceParent(serviceCollection)

makes far more sense than the way the IRC client in the current code is created and is much simpler, both to implement and to explain. It provides all of the same benefits of componentization discussed in the document, and it does so without using anything from twisted.python.components. Less is more.

There is certainly some place in the tutorial for coverage of twisted.python.components, but it needs to more carefully thought out than it was when this part of the finger tutorial was written.

[kehander1]

finger19.tac with components removed (from 25499)

[kehander1]

Very well... Does this make any sense? If it does, I will use it as a basis for splitting the code up into libraries and re-writing components.xhtml, and then subsequent bits. (I might add that it's not nearly as interesting-looking as it was before - but maybe that's for the best.)

[exarkun]

Yea, this looks good. There are a few minor problems with the patch (name errors and things like that) which I will commit fixes for in a moment. Then hopefully all that should remain to do is propagate these changes to the later finger listings.

[exarkun]

Applied at r25830.

[kehander1]

Yikes, it's been a while. Does this still make sense?

I'm still not fond of the name "UserStatusTree" instead of "HTTPFactoryFromService". It just doesn't seem to be descriptive in this context.

[kehander1]

Finger19.tac split into libraries

[glyph]

Ignoring the last attachment, since I'm not really sure how it's supposed to relate, this (the contents of the branch) looks ready to merge to me. It's definitely an improvement. Of course, I think the finger tutorial could still use more work :) but the new text seems ready to close this ticket.

[exarkun]

Then hopefully all that should remain to do is propagate these changes to the later finger listings.

That's how the last attachment relates.

[kehander1]

Things kind of stalled when no one could think of a good way to propagate changes through multiple finger libraries. The way the tutorial is structured, making a change in one library necessitates a new file with a new name, which means everything else that depends on that library has to be changed to reference the new file.

[thijs]

Replying to kehander1:

Things kind of stalled when no one could think of a good way to propagate changes through multiple finger libraries. The way the tutorial is structured, making a change in one library necessitates a new file with a new name, which means everything else that depends on that library has to be changed to reference the new file.

jerub worked on unit tests for the documentation codelistings in #2205, which include the finger libraries as well, so that would be part of, or related to, this ticket as well. Could you check that out as well kehander1?

[kehander1]

That doesn't seem like it's going to solve the problem in question, though. I'd point out the relevant portion of the IRC logs, but I can't seem to get to them at the moment.

[moshez]

Same finger19-libs as before, but without the dos newlines

[moshez]

The finger19-libs attachment had two problems:

1. Dos newlines. I fixed that.
2. The finger service does stuff in init that should be done in startService (_read, specifically)

I did not fix the second problem. Please move that code into startService, and make sure that all services are added to the application.

Unmarking review, assigning back.

[thijs]

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

[binjured]

This does not appear to have an active maintainer anymore; I've unassigned it. Its ultimate end will be determined by the perceived usefulness of the Finger tutorial, I suppose.

[thijs]

Replying to moshez:

2. The finger service does stuff in init that should be done in startService (_read, specifically) I did not fix the second problem. Please move that code into startService, and make sure that all services are added to the application.

This second problem was fixed with #3375.

[exarkun]

Fixing authorship metadata.