Opened 3 years ago

Last modified 7 weeks ago

#5633 enhancement new

better endpoint reprs

Reported by: glyph Owned by: _sunu_
Priority: normal Milestone:
Component: core Keywords: endpoint
Cc: chriswwolfe@… Branch: branches/better-endpoint-reprs-5633-4
(diff, github, buildbot, log)
Author: ashfall, glyph Launchpad Bug:

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 chhabraamit 3 years ago.
5633.patch (7.6 KB) - added by chhabraamit 3 years ago.
test_endpoints2.py (2.7 KB) - added by chhabraamit 3 years ago.
5633.bugfix (950 bytes) - added by chhabraamit 3 years ago.
endpoint-repr.patch (13.6 KB) - added by _sunu_ 6 months ago.

Download all attachments as: .zip

Change History (26)

comment:1 Changed 3 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 3 years ago by chhabraamit

Changed 3 years ago by chhabraamit

Changed 3 years ago by chhabraamit

comment:2 Changed 3 years ago by chhabraamit

  • Keywords review added

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

comment:3 follow-up: Changed 3 years ago by chhabraamit

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

comment:4 in reply to: ↑ 3 Changed 3 years ago by chhabraamit

Replying to chhabraamit:

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

And also commented twice .!!:P

comment:5 Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to chhabraamit

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 2 years ago by ashfall

  • Author set to ashfall
  • Branch set to branches/better-endpoint-reprs-5633

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

comment:7 Changed 2 years ago by ashfall

  • Owner changed from chhabraamit to ashfall

comment:8 Changed 2 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 2 years ago by exarkun

  • 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 2 years ago by ashfall

  • Branch changed from branches/better-endpoint-reprs-5633 to branches/better-endpoint-reprs-5633-2

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

comment:11 Changed 2 years ago by ashfall

  • Branch changed from branches/better-endpoint-reprs-5633-2 to branches/better-endpoint-reprs-5633-3

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

comment:12 Changed 19 months ago by ashfall

  • Owner ashfall deleted

comment:13 Changed 19 months ago by itamar

  • Keywords easy removed

comment:14 Changed 18 months ago by Julian

#4970 was a duplicate of this.

Changed 6 months ago by _sunu_

comment:15 Changed 6 months ago by _sunu_

  • Keywords review added
  • Owner set to _sunu_
  • Status changed from new to assigned

comment:16 Changed 6 months ago by exarkun

  • Keywords changed from endpoint, review to endpoint 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 follow-up: Changed 6 months 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 6 months ago by _sunu_

  • Owner _sunu_ deleted
  • Status changed from assigned to new

comment:19 in reply to: ↑ 17 Changed 6 months 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 months ago by glyph

  • Author changed from ashfall to ashfall, glyph
  • Branch changed from branches/better-endpoint-reprs-5633-3 to branches/better-endpoint-reprs-5633-4

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

comment:21 Changed 3 months ago by glyph

Put the patch into a branch and kicked the buildbots.

comment:22 Changed 7 weeks ago by herrwolfe

  • Cc chriswwolfe@… 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.