Ticket #5937 enhancement closed fixed

Opened 9 months ago

Last modified 3 months ago

Check current Cooperator status

Reported by: chris- Owned by: jml
Priority: normal Milestone:
Component: core Keywords:
Cc: Branch: branches/cooperator-running-5937
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

cooperator_running.patch Download (1.5 KB) - added by chris- 9 months ago.

Change History

Changed 9 months ago by chris-

1

  Changed 9 months ago by chris-

  • keywords review added

2

  Changed 5 months ago by jml

  • owner set to jml
  • keywords review removed

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.

3

  Changed 5 months ago by exarkun

What about also adding some documentation for this feature?

4

  Changed 5 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.

5

  Changed 4 months ago by jml

Filed #6213 for docs for Cooperator.

6

  Changed 4 months ago by jml

  • branch set to branches/cooperator-running-5937
  • branch_author set to jml

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

7

  Changed 4 months ago by jml

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

8

  Changed 4 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)

9

  Changed 4 months ago by jml

  • owner jml deleted
  • keywords review added

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.

10

  Changed 3 months ago by tom.prince

  • keywords review removed
  • owner set to jml
  • branch_author changed from jml to chris-, 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.

11

  Changed 3 months ago by jml

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

12

  Changed 3 months ago by jml

  • owner changed from jml to tom.prince
  • keywords review added

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

13

  Changed 3 months ago by tom.prince

  • owner changed from tom.prince to jml
  • keywords review removed
  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.

14

  Changed 3 months ago by jml

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

15

  Changed 3 months ago by jml

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

16

follow-up: ↓ 17   Changed 3 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)?

17

in reply to: ↑ 16 ; follow-up: ↓ 18   Changed 3 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.

18

in reply to: ↑ 17   Changed 3 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.

19

  Changed 3 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`.

20

  Changed 3 months ago by tom.prince

  • owner changed from tom.prince to jml
  • keywords review removed

Looks good, merge it.

21

  Changed 3 months ago by jml

  • status changed from new to closed
  • resolution set to fixed

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