Opened 15 months ago

Closed 4 months ago

#7013 enhancement closed wontfix (wontfix)

twisted.python.threadpool should comply with whitespace and coding standards

Reported by: hawkowl Owned by: hawkowl
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Branch: branches/threadpool-coding-standards-7013
(diff, github, buildbot, log)
Author: hawkowl Launchpad Bug:

Description

#7011 made me see that there could be some improvements in here.

Change History (8)

comment:1 Changed 15 months ago by hawkowl

  • Author set to hawkowl
  • Branch set to branches/threadpool-coding-standards-7013

(In [41789]) Branching to threadpool-coding-standards-7013.

comment:2 Changed 15 months ago by hawkowl

  • Keywords documentation review added
  • Owner hawkowl deleted

Putting it up for review - hopefully someone with more knowledge of ThreadPool can make sure that my documentation is correct (I've been reading the code and I think it's right, but that's what review is for!)

comment:3 Changed 15 months ago by adiroiban

  • Keywords review removed
  • Owner set to hawkowl

Thanks for the changes. Look good. Only minor comments while I exercise my review skills :)

# Create enough, but not too many

Not your code, but now that we are on it, do we need this comment since the docstring already talks about it.

log.msg('queue: %s'   % (self.q.queue,))

Just asking since this about coding standards. Is this the coding convention ? I was expecting:

log.msg('queue: %s' % (self.q.queue,))

Not sure if stopAWorker is stopping the next working that is finnished.... maybe WorkerStop marker should be renamed to StopNextFinnishedWorker ... or add documentation for WorkerStop

Other than that, all is good. Thanks!

Last edited 4 months ago by adiroiban (previous) (diff)

comment:4 Changed 4 months ago by hawkowl

Thanks, Adi.

I fixed up that little spacing issue, and I'm forcing a build now.

comment:5 Changed 4 months ago by hawkowl

  • Keywords review added

Okay, builds are passing.

It's now TwistedChecker and Pyflakes clean, except for TwistedChecker bugs (dict is a valid thing, twistedchecker!) and a scoping issue in a while block that pyflakes doesn't seem to think is being used afterwards, although the next while block will use it, maybe. It's a valid warning, because it's a bit icky, but isn't really in the scope of this ticket. There's also green across the board with buildbots so far.

Putting up for review, in case someone with ThreadPool knowledge specifically can say that the docstrings are correct.

comment:6 Changed 4 months ago by exarkun

is this going to conflict massively with the threadpool rewrite underway for #2673?

comment:7 Changed 4 months ago by hawkowl

Yeah. Yeah it is :(.

Looks like Glyph has fixed up all the docs in that, along with making most of the stuff in this outdated.

Oh well, guess that's what happens when you forget about a review for... uh, a year. Guess I'll close this one?

comment:8 Changed 4 months ago by glyph

  • Keywords review removed
  • Resolution set to wontfix
  • Status changed from new to closed

Sorry, hawkowl :(. I will endeavor to do something about #2673 this week. Bug me if I haven't.

Note: See TracTickets for help on using tickets.