Opened 6 years ago

Last modified 3 years ago

#5633 enhancement new

better endpoint reprs

Reported by: Glyph Owned by: _sunu_
Priority: normal Milestone:
Component: core Keywords: endpoint
Cc: Chris Wolfe Branch: branches/better-endpoint-reprs-5633-4
branch-diff, diff-cov, branch-cov, buildbot
Author: ashfall, glyph

Description

It is often desirable to indicate where one is going to be connecting (let's say, in a log message) before one actually makes such a connection. Given that most attributes of endpoints are private (and, for these purposes, should remain so: we don't want to have IStreamClientEndpoint grow a host attribute which is just a made-up value for logging), it is annoying that the repr() and str() of these endpoints just give a memory address, rather than information about where they will be listening on or connecting to.

(It may be worthwhile to split this into one ticket per endpoint type to have nice small branches.)

Attachments (5)

5633.2.patch (7.6 KB) - added by Amit Chhabra 6 years ago.
5633.patch (7.6 KB) - added by Amit Chhabra 6 years ago.
test_endpoints2.py (2.7 KB) - added by Amit Chhabra 6 years ago.
5633.bugfix (950 bytes) - added by Amit Chhabra 6 years ago.
endpoint-repr.patch (13.6 KB) - added by _sunu_ 4 years ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 6 years ago by Glyph

Keywords: easy added

I think this ticket is actually pretty easy; it's just making the attributes of the various classes in twisted/internet/endpoints.py visible in the results of their __repr__s. I'm adding the keyword after double-checking with #twisted.

Changed 6 years ago by Amit Chhabra

Attachment: 5633.2.patch added

Changed 6 years ago by Amit Chhabra

Attachment: test_endpoints2.py added

Changed 6 years ago by Amit Chhabra

Attachment: 5633.bugfix added

comment:2 Changed 6 years ago by Amit Chhabra

Keywords: review added

I mistakenly uploaded the same file twice , Sorry for that.

comment:3 Changed 6 years ago by Amit Chhabra

I mistakenly uploaded the same file twice , Sorry for that.

comment:4 in reply to:  3 Changed 6 years ago by Amit Chhabra

Replying to chhabraamit:

I mistakenly uploaded the same file twice , Sorry for that.

And also commented twice .!!:P

comment:5 Changed 6 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to Amit Chhabra

Thanks!

A few comments about the patch:

  1. Don't add new test files like test_existingthing2.py. 2 isn't really meaningful. If there's a good reason to have new tests in a new file, then it should be possible to come up with a good name for the new file. If you're just adding more tests for existing classes or functions, then it's almost certainly better to just add those tests to the existing test module.
  2. The Twisted coding standard calls for:
    1. lines to be wrapped before column 80
    2. whitespace around operators (eg foo % bar, not foo%bar)
    3. whitespace after , (eg (foo, bar) not (foo,bar))
    4. All modules, classes, and functions to have docstrings. This is particularly important for tests, where the docstring needs to convey to future maintainers the intent of the test.
    5. Test methods to be named like test_foo, not testFoo or test_Foo.
  3. twisted.internet is part of Twisted Core. The Twisted Core topfiles directory is twisted/topfiles, so the news fragment belongs there, rather than in twisted/internet/topfiles.
  4. The news fragment is a bit verbose. For a minor feature like this that touches a lot of classes in a single module, or which implement a single interface, it's probably better to summarize more. In particular, avoiding the repetition is very desirable. News fragments are half advertising material, half changelog entry. The point is to let people know very quickly what is different in a new release. We should avoid making them read essentially the same thing 8 times in a row.
  5. As far as the actual repr additions goes:
    1. The implementation is somewhat redundant. twisted.python.util.FancyStrMixin should help with this.
    2. Some of the things included in these reprs should not be included - they don't convey anything meaningful to the reader. For example, the reactor and the ssl context factory.
    3. Some parts of the behavior are not well tested. For example, passing "test1" as the ssl context factory when testing the SSL endpoint repr obscures the result that will happen in real usage. Passing an actual context factory would be more realistic, and probably also reveal that context factories do not have very good repr.
    4. The changes to _WrappingFactory and _WrappingProtocol seem out of scope, at least. I'm also not sure they benefit anyone. When would someone want to see the repr of one of these?

Thanks again for this! Sorry about the delay in the review.

comment:6 Changed 5 years ago by ashfall

Author: ashfall
Branch: branches/better-endpoint-reprs-5633

(In [35006]) Branching to 'better-endpoint-reprs-5633'

comment:7 Changed 5 years ago by ashfall

Owner: changed from Amit Chhabra to ashfall

comment:8 Changed 5 years ago by ashfall

Keywords: endpoint review added
Owner: ashfall deleted

Build Results

The news file may need improvement, I'm running short of ideas to phrase it well.

comment:9 Changed 5 years ago by Jean-Paul Calderone

Keywords: review removed
Owner: set to ashfall

Thanks.

  1. All of the classes that were switched from subclassing object to FancyStrMixin need to be switched to subclass both. :) FancyStrMixin is a classic class, so subclasses are also classic unless they also subclass a new-style class, such as object. Classic vs new-style is a stupid, boring distinction, but all new classes in Twisted must be new-style, and any existing classes in Twisted must be kept in their current style, be that classic or new-style. I forget whether class Foo(object, FancyStrMixin): or class Foo(FancyStrMixin, object): is more correct. You might find out through experimentation.
  2. There should probably be a test for the above. See twisted.internet.test.test_core.ObjectModelIntegrationMixin.assertFullyNewStyle and the following ObjectModelIntegrationTest.test_newstyleReactor for inspiration on how endpoints might be tested for this feature.
  3. Feel free to call the builtin repr in the unit tests, instead of the __repr__ method of each endpoint.
  4. I think the reprs would all be improved by the removal of the leading _ on almost every attribute they display. You can do this by putting more information into the showAttributes attribute. Its elements may be strings, as they are now, or tuples like (attributeName, displayName, formatCharacter). displayName will let you clean up the display.
  5. Also, I suggest displaying the UNIX endpoint's mode in octal instead of decimal, since octal is the convention for dealing with that value.

comment:10 Changed 5 years ago by ashfall

Branch: branches/better-endpoint-reprs-5633branches/better-endpoint-reprs-5633-2

(In [35263]) Branching to 'better-endpoint-reprs-5633-2'

comment:11 Changed 5 years ago by ashfall

Branch: branches/better-endpoint-reprs-5633-2branches/better-endpoint-reprs-5633-3

(In [35705]) Branching to 'better-endpoint-reprs-5633-3'

comment:12 Changed 5 years ago by ashfall

Owner: ashfall deleted

comment:13 Changed 5 years ago by Itamar Turner-Trauring

Keywords: easy removed

comment:14 Changed 5 years ago by Julian Berman

#4970 was a duplicate of this.

Changed 4 years ago by _sunu_

Attachment: endpoint-repr.patch added

comment:15 Changed 4 years ago by _sunu_

Keywords: review added
Owner: set to _sunu_
Status: newassigned

comment:16 Changed 4 years ago by Jean-Paul Calderone

Keywords: endpoint, reviewendpoint review

Thanks! For future reference, note that trac uses spaces to separate keywords, not commas. Also, when attaching a patch, please mention what the patch is based on! (trunk, the branch referenced by the ticket, an earlier patch, etc)

comment:17 Changed 4 years ago by _sunu_

Thanks for the suggestions. This is my first time using Trac.

What exactly does "what the patch is based on" refer to? Is it the branch to which the patch should be applied or the branch or patch from which I took inspiration for the changes? In the first case it is the trunk, but in the second case it's better-endpoint-reprs-5633-3 branch.

Also, how can I add a new branch instead of adding a patch?

comment:18 Changed 4 years ago by _sunu_

Owner: _sunu_ deleted
Status: assignednew

comment:19 in reply to:  17 Changed 4 years ago by Glyph

Replying to _sunu_:

Thanks for the suggestions. This is my first time using Trac.

What exactly does "what the patch is based on" refer to? Is it the branch to which the patch should be applied or the branch or patch from which I took inspiration for the changes?

It is the branch (and revision, if applicable) from which you ran your diff command. Which should also be the branch to which you expect the patch to be applied.

In the first case it is the trunk, but in the second case it's better-endpoint-reprs-5633-3 branch.

Thanks for answering both questions ;). So you included the changes from the better-endpoint-reprs-5633-3 branch?

Also, how can I add a new branch instead of adding a patch?

Our current development process dictates that external (i.e. non-committer) developers need to submit patches. The reasons for this are kind of boring and hopefully we can fix it eventually, but nevertheless fixing it is a chunk of work that nobody has had time to do. So you can get commit, but getting commit requires contributing a few changes and generally demonstrating the ability to follow the process.

One of the committers will put your changes into a branch in SVN when they review it.

However, if, in addition to the patch, you want to also provide a link to a git branch that you've publicly hosted which matches the patch, that can always be helpful!

comment:20 Changed 3 years ago by Glyph

Author: ashfallashfall, glyph
Branch: branches/better-endpoint-reprs-5633-3branches/better-endpoint-reprs-5633-4

(In [42970]) Branching to better-endpoint-reprs-5633-4.

comment:21 Changed 3 years ago by Glyph

Put the patch into a branch and kicked the buildbots.

comment:22 Changed 3 years ago by Chris Wolfe

Cc: Chris Wolfe added
Keywords: review removed
Owner: set to _sunu_

Thanks for you patch!

All of the new reprs look good to me, as do all of the new tests. The only issue I have found is one which exarkun pointed out: since FancyStrMixin is an old-style class, any of the endpoints inheriting from it need to be tested to ensure that they are indeed still new-style classes.

The mixin and method twisted.internet.test.test_core.ObjectModelIntegrationMixin.assertFullyNewStyle can be used inside of a unit test to assert that a class is a fully new style class. See ObjectModelIntegrationTest.test_newstyleReactor for examples of how this mixin can be used.

Each of the endpoints that has changed should be tested for being fully new style. Once the new tests have been written, please resubmit your patch for review.

(Also please bear with me, this is my first review)

Note: See TracTickets for help on using tickets.