Opened 6 years ago

Closed 6 years ago

#3197 defect closed fixed (fixed)

IProcessTransport.pid is incompletely documented

Reported by: exarkun Owned by:
Priority: normal Milestone:
Component: core Keywords: documentation
Cc: thijs Branch: branches/pid-doc-3197-2
(diff, github, buildbot, log)
Author: thijs, exarkun Launchpad Bug:

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 exarkun 6 years ago.

Download all attachments as: .zip

Change History (20)

comment:1 Changed 6 years ago by thijs

  • Cc thijs added
  • Keywords documentation added
  • Owner changed from glyph to thijs
  • Status changed from new to assigned

comment:2 Changed 6 years ago by thijs

  • author set to thijs
  • Branch set to branches/pid-doc-3197

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

comment:3 Changed 6 years ago by thijs

  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

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

comment:4 Changed 6 years ago by exarkun

  • Keywords review removed
  • Owner set to thijs

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 6 years ago by thijs

  • Status changed from new to assigned

comment:6 Changed 6 years ago by thijs

  • Keywords review added
  • Owner thijs deleted
  • Status changed from assigned to new

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 6 years ago by exarkun

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 6 years ago by exarkun

comment:8 Changed 6 years ago by exarkun

  • author changed from thijs to thijs, exarkun

comment:9 follow-up: Changed 6 years ago by glyph

  • Keywords review removed
  • Owner set to exarkun

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 6 years ago by thijs

  • Owner changed from exarkun to thijs
  • Status changed from new to assigned

comment:11 Changed 6 years ago by thijs

  • Branch changed from branches/pid-doc-3197 to branches/pid-doc-3197-2

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

comment:12 in reply to: ↑ 9 Changed 6 years ago by thijs

  • Owner changed from thijs to exarkun
  • Status changed from assigned to new

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 branch and applied the patch. Assigning it to exarkun so he can possibly rephrase it and then merge.

comment:13 Changed 6 years ago by exarkun

(In [25324]) another approach

refs #3197

comment:14 Changed 6 years ago by exarkun

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

refs #3197

comment:15 Changed 6 years ago by exarkun

  • Keywords review added
  • Owner exarkun deleted

comment:16 Changed 6 years ago by radix

  • Keywords review removed
  • Owner set to exarkun

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 6 years ago by exarkun

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 6 years ago by exarkun

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

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

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

Expand the interface documentation for IProcessTransport.pid.

comment:19 Changed 3 years ago by <automation>

  • Owner exarkun deleted
Note: See TracTickets for help on using tickets.