[Twisted-Python] Changing procmon.ProcessMonitor.processes
Hello all, For context, please read bullet one of Glyph's comment on https://twistedmatrix.com/trac/ticket/3691#comment:9 I am in a similar situation to JP 8 years ago -- I want to add a working directory argument to addProcess, so that processes that care (usually for silly reasons) about their cwd can be monitored. The options I see: 1. Move processes to a private attribute, deprecate processors, then do it. 2. Have a parallel dictionary to processes 3. Move, as Glyph suggested, to object with attributes. For backwards compatibility, I can support __getitem__ for the historic unpackers. 4. Decide we're going with "inccompatible change", and move processes to a private attribute. Anyone wants to weigh in?
The options I see:
1. Move processes to a private attribute, deprecate processors, then do it.
From an abstraction standpoint, that makes sense. Probably the same could be said about the other process related dict attributes like protocols, delay, etc., though. They all seem to be "name" indexed dicts of things related to a given process. In other words, moving processes into a private attribute suggests making all (most?) other attributes private as well.
2. Have a parallel dictionary to processes
Given that there are already lots of interrelated dicts, I'd say this would continue the current design which, as we're seeing now, is not easily extendable. I'd try to avoid this. Sidenote: A quick, mostly backwards compatible, change could probably add cwd to the existing per-process tuple in self.processes as an extra item, couldn't it? Not 100% backwards compatible, because the tuple would have one more element, but nearly 100%.
3. Move, as Glyph suggested, to object with attributes. For backwards compatibility, I can support __getitem__ for the historic unpackers.
This feels cleaner and better. Maybe __getitem__ could be implemented and deprecated immediately, who knows? Again, creating a class here could also induce doing something similar with the "process policy/state attributes" (for the lack of a better name), where maybe the other dicts could somehow be brought together. Whether this is a better design or not is up for consideration. But one thing is a fact, just bringing them together into a common class (and, thus, down to a single attribute in ProcessMonitor) would break backwards compatibility for some of the same reasons you are now raising. *sigh!*
4. Decide we're going with "incompatible change", and move processes to a private attribute.
I see no reason for accessing those attributes from "outside of ProcessMonitor" but if Twisted claims not to break existing code without previous deprecation warning, such an approach would break that promise (full policy details here http://twistedmatrix.com/documents/current/core/development/policy/compatibi...). My 2c. Regards, -- exvito
On Mon, Sep 11, 2017 at 2:44 AM ex vito <ex.vitorino@gmail.com> wrote:
Sidenote: A quick, mostly backwards compatible, change could probably add cwd to the existing per-process tuple in self.processes as an extra item, couldn't it?
No. Most usage of this tuple is unpacking, which makes it really really not backwards compatible.
4. Decide we're going with "incompatible change", and move processes to a private attribute.
I see no reason for accessing those attributes from "outside of ProcessMonitor" but if Twisted claims not to break existing code without previous deprecation warning, such an approach would break that promise (full policy details here http://twistedmatrix.com/documents/current/core/development/policy/compatibi... ).
Specifically, I was asking about http://twistedmatrix.com/documents/current/core/development/policy/compatibi...
On 2017-09-18, at 16:35, Moshe Zadka <zadka.moshe@gmail.com> wrote:
On Mon, Sep 11, 2017 at 2:44 AM ex vito <ex.vitorino@gmail.com> wrote: Sidenote: A quick, mostly backwards compatible, change could probably add cwd to the existing per-process tuple in self.processes as an extra item, couldn't it?
No. Most usage of this tuple is unpacking, which makes it really really not backwards compatible.
If that is the case, then I stand corrected. That would not be backwards compatible at all.
4. Decide we're going with "incompatible change", and move processes to a private attribute.
I see no reason for accessing those attributes from "outside of ProcessMonitor" but if Twisted claims not to break existing code without previous deprecation warning, such an approach would break that promise (full policy details here http://twistedmatrix.com/documents/current/core/development/policy/compatibi...).
Specifically, I was asking about http://twistedmatrix.com/documents/current/core/development/policy/compatibi...
We're on the same page, then (literally). Quoting that section, with regards to incompatible changes, "Generally, the reason that one would want to do this is to give applications a performance enhancement or bug fix that could break behavior that unanticipated, hypothetical uses of an existing API (...)". Would this change fall under that? Maybe not. Then again, I understand that "Every change is unique", so maybe those words don't need to be taken in a strict sense. Other than that, again, per that section's rules, not being a commiter myself, I'm in no position to approve such a change. I wonder, however, how "urgent" such a final change is to you and why a deprecation cycle does not fit your purpose (even though, admittedly, it may represent more effort). My 2c. -- exvito
On Tue, Sep 19, 2017 at 3:51 AM ex vito <ex.vitorino@gmail.com> wrote:
Other than that, again, per that section's rules, not being a commiter myself, I'm in no position to approve such a change. I wonder, however, how "urgent" such a final change is to you and why a deprecation cycle does not fit your purpose (even though, admittedly, it may represent more effort).
I think that between the low likelihood that someone went crawling over the attributes manually, the RoI of having a deprecation cycle with some intermediate solution that later needs to be cleaned up, and the fact that this would be a clean break (i.e., "AttributeError") rather than some obscure error, I am at least interested in opinions about going the exception route. ProcMon is non-trivial to productionize, and I'm not aware of anyone even using it in production, other than me, let alone crawling over its internal state. Moshe Z.
On 2017-09-19, at 15:49, Moshe Zadka <zadka.moshe@gmail.com> wrote:
On Tue, Sep 19, 2017 at 3:51 AM ex vito <ex.vitorino@gmail.com> wrote:
Other than that, again, per that section's rules, not being a commiter myself, I'm in no position to approve such a change. I wonder, however, how "urgent" such a final change is to you and why a deprecation cycle does not fit your purpose (even though, admittedly, it may represent more effort).
I think that between the low likelihood that someone went crawling over the attributes manually, the RoI of having a deprecation cycle with some intermediate solution that later needs to be cleaned up, and the fact that this would be a clean break (i.e., "AttributeError") rather than some obscure error, I am at least interested in opinions about going the exception route.
ProcMon is non-trivial to product ionize, and I'm not aware of anyone even using it in production, other than me, let alone crawling over its internal state.
Agreed. For completeness, the code I work with does not make use of ProcMon and I don't recall having ever used it.
OK I opened a ticket with a plan, after discussing with Mark Williams. 1. Make a custom class that implements the Sequence ABC and pretends to be sized the old size. 2. Internally all access will be moved to attributes 3. All sequence methods will be marked as Deprecated. We'll kill them in a year. 4. processes will become _processes, with a Deprecated .processes accessor, again for backwards compatibility. I am technically changing the class of a thing from a tuple to a different class, but I don't think that this can be in general relied on. The API docs do not mention the attribute, much less promise the concrete class of its contents. Objections? Ticket: https://twistedmatrix.com/trac/ticket/9287 On Wed, Sep 20, 2017 at 1:34 AM ex vito <ex.vitorino@gmail.com> wrote:
On 2017-09-19, at 15:49, Moshe Zadka <zadka.moshe@gmail.com> wrote:
On Tue, Sep 19, 2017 at 3:51 AM ex vito <ex.vitorino@gmail.com> wrote:
Other than that, again, per that section's rules, not being a commiter myself, I'm in no position to approve such a change. I wonder, however, how "urgent" such a final change is to you and why a deprecation cycle does not fit your purpose (even though, admittedly, it may represent more effort).
I think that between the low likelihood that someone went crawling over the attributes manually, the RoI of having a deprecation cycle with some intermediate solution that later needs to be cleaned up, and the fact that this would be a clean break (i.e., "AttributeError") rather than some obscure error, I am at least interested in opinions about going the exception route.
ProcMon is non-trivial to product ionize, and I'm not aware of anyone even using it in production, other than me, let alone crawling over its internal state.
Agreed. For completeness, the code I work with does not make use of ProcMon and I don't recall having ever used it.
_______________________________________________ Twisted-Python mailing list Twisted-Python@twistedmatrix.com https://twistedmatrix.com/cgi-bin/mailman/listinfo/twisted-python
On 2017-09-21, at 2:13, Moshe Zadka <zadka.moshe@gmail.com> wrote:
OK I opened a ticket with a plan, after discussing with Mark Williams.
1. Make a custom class that implements the Sequence ABC and pretends to be sized the old size. 2. Internally all access will be moved to attributes 3. All sequence methods will be marked as Deprecated. We'll kill them in a year. 4. processes will become _processes, with a Deprecated .processes accessor, again for backwards compatibility.
I am technically changing the class of a thing from a tuple to a different class, but I don't think that this can be in general relied on. The API docs do not mention the attribute, much less promise the concrete class of its contents.
Walks like a duck, quacks like a duck. Sounds like a clean approach.
Objections?
I'd say go for it. -- exvito
participants (2)
-
ex vito -
Moshe Zadka