Opened 4 years ago

Closed 3 years 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
branch-diff, diff-cov, branch-cov, buildbot
Author: hawkowl


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

Change History (8)

comment:1 Changed 4 years ago by hawkowl

Author: hawkowl
Branch: branches/threadpool-coding-standards-7013

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

comment:2 Changed 4 years 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 4 years ago by Adi Roiban

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 3 years ago by Adi Roiban (previous) (diff)

comment:4 Changed 3 years ago by hawkowl

Thanks, Adi.

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

comment:5 Changed 3 years 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 3 years ago by Jean-Paul Calderone

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

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

Keywords: review removed
Resolution: wontfix
Status: newclosed

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.