Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#5252 enhancement closed fixed (fixed)

Manhole should support CTRL-A and CTRL-E for home/end

Reported by: djfroofy Owned by: exarkun
Priority: normal Milestone:
Component: conch Keywords:
Cc: thijs Branch: branches/manhole-home-end-5252
(diff, github, buildbot, log)
Author: djfroofy Launchpad Bug:

Description

In a manhole instance, CTRL-A and CTRL-E should trigger HOME and END, respectively.

Attachments (1)

home-end-5252.patch (2.6 KB) - added by djfroofy 3 years ago.

Download all attachments as: .zip

Change History (17)

Changed 3 years ago by djfroofy

comment:1 Changed 3 years ago by djfroofy

  • Keywords review added

comment:2 Changed 3 years ago by thijs

  • Author set to djfroofy
  • Cc thijs added
  • Component changed from core to conch
  • Keywords review removed
  • Owner set to djfroofy

Thanks. Minor stuff:

  1. twisted/conch/topfiles/5252.bugfix has a typo
  2. The docstring of the Manhole class should be on 3 lines. While your at it, could you also correct this for the other docstrings in that module?

comment:3 follow-up: Changed 3 years ago by glyph

  • Owner changed from djfroofy to thijs

Hi thijs,

It's unclear if this is a complete review. Will this be ready to merge after those two points are addressed? If not, this really should have been a comment that didn't remove the 'review' keyword. If so, then you should say 'this should be ready to merge after these two points are addressed' or something similar.

I think that one of our various documents on the review process says something like this, but I can't find the reference at the moment. In any case it's helpful to contributors to know what the status of the review is and what the next step is whenever they get a feedback comment.

Thanks for doing some reviews,

-glyph

comment:4 in reply to: ↑ 3 Changed 3 years ago by thijs

  • Keywords review added
  • Owner thijs deleted

Replying to glyph:

Hi thijs,

It's unclear if this is a complete review.

Thanks for the feedback glyph. I guess it wasn't a complete review but before this gets merged I wouldn't want those 2 points to get unnoticed and fixed in the final merge. So I'm putting it back up for review for the remaining issues if any.

comment:5 Changed 3 years ago by djfroofy

Discussed some with exarkun on irc and adding this to manhole might not be the best pattern repeat. Perhaps using a widget-based approach (insults.window) will be easier to maintain long term. An example:

http://twistedmatrix.com/trac/browser/sandbox/exarkun/invective/trunk/invective/widgets.py

However my core motivation is simply make the thing you get when you type "python -m twisted.conch.stdio" (or better yet a successor twistd plugin) completely awesome. I'd appreciate any help in defining a road map towards this end. (This ticket probably is a bad start to the journey). Some ideas I have at a higher level:

  1. Create successor to conch.stdio that will be modeled a twistd plugin
  2. Leverage insults.window to provide more complex input widgets atop RecvLine (vs. hacking the inheritance chain).
  3. (Optionally) Deprecate stdio and advertise its successor as "the thing you use as a REPL for twisted"

Thoughts?

comment:6 Changed 3 years ago by djfroofy

(Correcting formatting in previous comment)

  1. Create successor to conch.stdio that will be modeled a twistd plugin
  2. Leverage insults.window to provide more complex input widgets atop RecvLine (vs. hacking the inheritance chain).
  3. (Optionally) Deprecate stdio and advertise its successor as "the thing you use as a REPL for twisted"

comment:7 Changed 3 years ago by exarkun

  • Author changed from djfroofy to exarkun, djfroofy
  • Branch set to branches/manhole-home-end-5252

(In [32552]) Branching to 'manhole-home-end-5252'

comment:8 Changed 3 years ago by exarkun

(In [32553]) Apply home-end-5252.patch

refs #5252

comment:9 Changed 3 years ago by exarkun

(In [32554]) Apply the other part of it

refs #5252

comment:10 Changed 3 years ago by exarkun

  • Author changed from exarkun, djfroofy to djfroofy

comment:11 follow-up: Changed 3 years ago by exarkun

  • Keywords review removed
  • Owner set to exarkun

(Unsurprisingly) I agree that we should have an awesome REPL, and as I discussed with djfroofy on IRC I think twisted.conch.manhole probably isn't the best approach we can take to that end.

I don't think we should spend a lot more time enhancing twisted.conch.manhole.Manhole. However, this is a pretty simple patch which looks fine to me, so I don't see why we shouldn't apply it.

Separately, I encourage people interested in this area to write down some ideas about bigger plans, and I'm happy to contribute my thoughts on the topic as well.

Just a couple nits with this patch, which I'll correct before I merge the branch:

  1. We like two blank lines between methods
  2. Adding HOME and END support is probably a feature enhancement rather than a bug fix, so 5252.bugfix should probably be 5252.feature :)

Thanks!

comment:12 Changed 3 years ago by exarkun

(In [32555]) Insert desirable vertical whitespace; trivial docstring tweak.

refs #5252

comment:13 in reply to: ↑ 11 Changed 3 years ago by glyph

Replying to exarkun:

I don't think we should spend a lot more time enhancing twisted.conch.manhole.Manhole. However, this is a pretty simple patch which looks fine to me, so I don't see why we shouldn't apply it.

It seems like it's so close to being acceptable, though. It seems like a few more small tweaks like this one would be worthwhile, if not a massive additional investment of effort. Once we have something acceptable in place, perhaps the next revision should be architected differently?

In any case - can you put that IRC discussion somewhere, perhaps just pasted into a wiki page, for future reference? I'm not familiar with the reasons for not wanting to enhance it, or what else should be done, and it seems like that might be a start.

comment:14 Changed 3 years ago by exarkun

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

(In [32557]) Merge manhole-home-end-5252

Author: djfroofy
Reviewer: exarkun
Fixes: #5252

Add C-a and C-e support for home and end functions to twisted.conch.manhole.

comment:15 Changed 3 years ago by exarkun

I created Manhole?.

comment:16 Changed 3 years ago by exarkun

nope. plan/Manhole.

Note: See TracTickets for help on using tickets.