Opened 5 years ago

Closed 4 years ago

#5937 enhancement closed fixed (fixed)

Check current Cooperator status

Reported by: Christian Kampka Owned by: Jonathan Lange
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/cooperator-running-5937
branch-diff, diff-cov, branch-cov, buildbot
Author: chris-, jml

Description

It is occasionally useful to know the current running status of a Cooperator. Everything is there except a public API. Attached patch remedies this.

Attachments (1)

cooperator_running.patch (1.5 KB) - added by Christian Kampka 5 years ago.

Download all attachments as: .zip

Change History (22)

Changed 5 years ago by Christian Kampka

Attachment: cooperator_running.patch added

comment:1 Changed 5 years ago by Christian Kampka

Keywords: review added

comment:2 Changed 4 years ago by Jonathan Lange

Keywords: review removed
Owner: set to Jonathan Lange

Thanks for the patch!

A few tweaks:

  1. The first line of the docstring for test_running is redundant. Also, as a matter of personal taste, phrasing without "should" can make test docstrings read better.
  1. In the news patch, "determin" should be "determine".

I'm happy to make these changes and land the patch. I won't be able to for another 5-6 hours though.

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

What about also adding some documentation for this feature?

comment:4 Changed 4 years ago by Jonathan Lange

I think the docstring in the patch is enough for this feature.

I looked, but couldn't find any existing docs on Cooperator to update. Adding such docs deserves its own ticket.

comment:5 Changed 4 years ago by Jonathan Lange

Filed #6213 for docs for Cooperator.

comment:6 Changed 4 years ago by Jonathan Lange

Author: jml
Branch: branches/cooperator-running-5937

(In [37065]) Branching to 'cooperator-running-5937'

comment:7 Changed 4 years ago by Jonathan Lange

(In [37067]) Docstring and news changes as per review (refs #5937)

comment:8 Changed 4 years ago by Jonathan Lange

(In [37068]) Split the 'running' test up into three tests.

Because that's how I roll (also, intermediate assertions aren't a great thing)

(refs #5937)

comment:9 Changed 4 years ago by Jonathan Lange

Keywords: review added
Owner: Jonathan Lange deleted

Applied notes from my earlier review. Once the tests were up in my editor I couldn't help splitting them up, so this needs another review.

comment:10 Changed 4 years ago by Tom Prince

Author: jmlchris-, jml
Keywords: review removed
Owner: set to Jonathan Lange
  1. From reading the ticket, I assumed that .running would be an attribute. Also, there are already a number of places (LoopingCall which is also in t.i.task comes to mind) which have .running as attribute.
    • This involves turning it into a property.
  2. I think it would reasonable to have tests of running when the default value of started=True is passed.

Please resubmit for review after addressing the above points.

comment:11 Changed 4 years ago by Jonathan Lange

(In [37145]) Make running a property, as per review item 1. refs #5937.

comment:12 Changed 4 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Tom Prince

Thanks for the review!

  1. Addressed in [37145] by making it a property. Note the docstring change and the use of the rare (but now permitted?) decorator syntax.
  2. Addressed in [37144].

I removed a pyflakes error from another test in the file while I was at it [37143].

comment:13 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Jonathan Lange
  1. Please restore the @return annotation and add an @rtype annotation. Pydoctor currently documents properties as methods (see generated docs) and even when it supports properties, it should be able to deal with @return.
  2. I missed this on my first review, assertTrue and assertFalse should be preferred when appropriate.

Please merge after addressing the above points.

comment:14 Changed 4 years ago by Jonathan Lange

(In [37151]) Use @return, because pydoctor prefers it. refs #5937

comment:15 Changed 4 years ago by Jonathan Lange

(In [37152]) Use assertTrue and assertFalse instead of assertEquals. refs #5937

comment:16 Changed 4 years ago by Jonathan Lange

Keywords: review added
Owner: changed from Jonathan Lange to Tom Prince

Thanks for the quick turnaround. I've made the changes as you suggest.

Where does it say that assertTrue(x) is preferable to assertEqual(x, True)?

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

Replying to jml:

Where does it say that assertTrue(x) is preferable to assertEqual(x, True)?

If this is official policy, we might want to just change the policy. assertEqual(x, True) produces a nicer failure message.

comment:18 in reply to:  17 Changed 4 years ago by Jonathan Lange

Replying to glyph:

Replying to jml:

Where does it say that assertTrue(x) is preferable to assertEqual(x, True)?

If this is official policy, we might want to just change the policy. assertEqual(x, True) produces a nicer failure message.

I think it is too small a thing to be in a policy at all.

comment:19 Changed 4 years ago by Tom Prince

Or make assertTrue produce a better message? I've noticed that python 2.7's TestCase produces nicer message than trial's, at least for assertTrue and assertEqual`.

comment:20 Changed 4 years ago by Tom Prince

Keywords: review removed
Owner: changed from Tom Prince to Jonathan Lange

Looks good, merge it.

comment:21 Changed 4 years ago by Jonathan Lange

Resolution: fixed
Status: newclosed

(In [37153]) Merge cooperator-running-5937: Add running property to Cooperator

Author: chris-, jml Reviewers: jml, tom.prince Fixes: #5937

Cooperator now has a boolean property, running, which can be used to determine whether or not that cooperator is running.

Note: See TracTickets for help on using tickets.