Opened 2 years ago

Closed 20 months ago

#5937 enhancement closed fixed (fixed)

Check current Cooperator status

Reported by: chris- Owned by: jml
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/cooperator-running-5937
(diff, github, buildbot, log)
Author: chris-, jml Launchpad Bug:

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 chris- 2 years ago.

Download all attachments as: .zip

Change History (22)

Changed 2 years ago by chris-

comment:1 Changed 2 years ago by chris-

  • Keywords review added

comment:2 Changed 22 months ago by jml

  • Keywords review removed
  • Owner set to jml

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 22 months ago by exarkun

What about also adding some documentation for this feature?

comment:4 Changed 22 months ago by jml

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 20 months ago by jml

Filed #6213 for docs for Cooperator.

comment:6 Changed 20 months ago by jml

  • Author set to jml
  • Branch set to branches/cooperator-running-5937

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

comment:7 Changed 20 months ago by jml

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

comment:8 Changed 20 months ago by jml

(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 20 months ago by jml

  • Keywords review added
  • Owner jml 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 20 months ago by tom.prince

  • Author changed from jml to chris-, jml
  • Keywords review removed
  • Owner set to jml
  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 20 months ago by jml

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

comment:12 Changed 20 months ago by jml

  • Keywords review added
  • Owner changed from jml 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 20 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to jml
  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 20 months ago by jml

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

comment:15 Changed 20 months ago by jml

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

comment:16 follow-up: Changed 20 months ago by jml

  • Keywords review added
  • Owner changed from jml 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 ; follow-up: Changed 20 months 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 20 months ago by jml

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 20 months 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 20 months ago by tom.prince

  • Keywords review removed
  • Owner changed from tom.prince to jml

Looks good, merge it.

comment:21 Changed 20 months ago by jml

  • Resolution set to fixed
  • Status changed from new to closed

(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.