Opened 10 years ago

Closed 9 years ago

#3197 defect closed fixed (fixed)

IProcessTransport.pid is incompletely documented

Reported by: Jean-Paul Calderone Owned by:
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: Thijs Triemstra Branch: branches/pid-doc-3197-2
branch-diff, diff-cov, branch-cov, buildbot
Author: thijs, exarkun

Description

After a process ends, its pid attribute is set to None. The interface doesn't document this, or the type of pid while the process is running.

Attachments (1)

processtransport-pid.patch (671 bytes) - added by Jean-Paul Calderone 9 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 9 years ago by Thijs Triemstra

Cc: Thijs Triemstra added
Keywords: documentation added
Owner: changed from Glyph to Thijs Triemstra
Status: newassigned

comment:2 Changed 9 years ago by Thijs Triemstra

author: thijs
Branch: branches/pid-doc-3197

(In [24893]) Branching to 'pid-doc-3197'

comment:3 Changed 9 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Added the None docstring in r24894, but still need to know the 'type of pid while the process is running'.

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

Keywords: review removed
Owner: set to Thijs Triemstra

It's an int while it's not None. It's worth mentioning that it is set to None before processEnded is called, I think.

comment:5 Changed 9 years ago by Thijs Triemstra

Status: newassigned

comment:6 Changed 9 years ago by Thijs Triemstra

Keywords: review added
Owner: Thijs Triemstra deleted
Status: assignednew

Updated the docstring in r25029. I manually merged forward, creating a new branch is too much for such a small change imo.

comment:7 Changed 9 years ago by Jean-Paul Calderone

Small change, but it conflicts if I try to merge it with trunk. I'm not sure what a "manual merge forward" is, but since it doesn't prevent conflicts I think this needs a new branch, or at least attach a patch with the resolved diff to the ticket. Actually, I'll do that.

Changed 9 years ago by Jean-Paul Calderone

Attachment: processtransport-pid.patch added

comment:8 Changed 9 years ago by Jean-Paul Calderone

author: thijsthijs, exarkun

comment:9 Changed 9 years ago by Glyph

Keywords: review removed
Owner: set to Jean-Paul Calderone

This is an improvement, it should be merged. It's a bit awkwardly worded though - it implies that after processEnded it called, pid is set to something different.

comment:10 Changed 9 years ago by Thijs Triemstra

Owner: changed from Jean-Paul Calderone to Thijs Triemstra
Status: newassigned

comment:11 Changed 9 years ago by Thijs Triemstra

Branch: branches/pid-doc-3197branches/pid-doc-3197-2

(In [25316]) Branching to 'pid-doc-3197-2'

comment:12 in reply to:  9 Changed 9 years ago by Thijs Triemstra

Owner: changed from Thijs Triemstra to Jean-Paul Calderone
Status: assignednew

Replying to glyph:

This is an improvement, it should be merged. It's a bit awkwardly worded though - it implies that after processEnded it called, pid is set to something different.

That's what it implies but what should it say otherwise?

Created a new [source:branches/pid-doc-3197-2 branch] and applied the patch. Assigning it to exarkun so he can possibly rephrase it and then merge.

comment:13 Changed 9 years ago by Jean-Paul Calderone

(In [25324]) another approach

refs #3197

comment:14 Changed 9 years ago by Jean-Paul Calderone

(In [25325]) s/C{/L{/

refs #3197

comment:15 Changed 9 years ago by Jean-Paul Calderone

Keywords: review added
Owner: Jean-Paul Calderone deleted

comment:16 Changed 9 years ago by radix

Keywords: review removed
Owner: set to Jean-Paul Calderone

I thought we don't use L{} for built-in types and None?

If we do, then merge. If we don't, change to C{} and merge.

comment:17 Changed 9 years ago by Jean-Paul Calderone

Lately, I feel like L{} means fully-qualified Python name and C{} means evaluatable-ish expression. So I guess I'll leave it as-is.

comment:18 Changed 9 years ago by Jean-Paul Calderone

Resolution: fixed
Status: newclosed

(In [25367]) Merge pid-doc-3197-2

Author: thijs, exarkun Reviewer: radix Fixes: #3197

Expand the interface documentation for IProcessTransport.pid.

comment:19 Changed 7 years ago by <automation>

Owner: Jean-Paul Calderone deleted
Note: See TracTickets for help on using tickets.