Minimal scaffolding for feature-gated Helix Mode implementation.#1033
Minimal scaffolding for feature-gated Helix Mode implementation.#1033fdncred merged 1 commit intonushell:mainfrom
Conversation
|
I think this is a good start. I'm going to ping the core-team on this, so they know what's going on and try to get more buy-in. |
|
We've got good buy-in on this PR from the core-team. We just need to keep having bite-sized PRs I think. |
|
Would you guys be willing to look at a stacked PR setup for this feature? I think it would make everyones jobs a bit easier on both sides of it... encourage smaller PRs and more frequent reviews. If that would be helpful i can look into what the lowest-barrier tool would be to facilitate that. If not, just a little guidance on what expectations are for PR surface area should be fine |
|
I'm open but what do you mean by stacked PR do you just mean a PR with a bunch of sound commits or what? To elaborate more, what make reviewing large PRs tremendously difficult is:
We definitely need to grok the changes and avoid force pushes as much as possible. I agree that "small" and "large" are vague and I don't have a metric to share other than to say #960 is large, not because of the amount of files changed, but because of what's going on and #1033 is small because of the amount of changes. I'm happy to work with people to make it easy on both parties, authors and reviewers. |
Stacked PR generally means creating PR of a branch that isnt merged yet. Like you make branch Btw, I am not entirely sure if I got this correct. But thats what I read about stacked pull request some time ago. |
|
I don't have a problem with PRs that are dependent on each other as long as i know the order they need to land in, then it's fine. |
|
Stacked PRs are a little different. They are a sequence of PRs usually they are either mandated to be single-comment or are squashed every review. You "stack" the PRs by defining the chain of depedencies between successive PRs. The main advantage is that you can submit ready-for-review chunks of a feature while continuing further work on the feature simultaneously. You can see how this would encourage smaller and more frequent PRs. If the first PR on the stack looks good without changes, you can either merge right then and there or wait for the entire "stack" to be done before a final full-feature merge. The real advantage of this approach is if someone has feedback on an "early" part of the stack that requires changes; you'll either need to make one or a dozen dumb fix commits, or alternatively you'll have a doozy of a time with merge/rebase conflicts and refactoring challenges if your follow-up work is in larger chunks. What the stacked diff tooling ecosystem does to alleviate some of this pain (see the Tools for Stacking section at https://www.stacking.dev/) is
**All this is just one of many reasons more and more people are moving to jujutsu 🤪 The stacked-pr tools try to deal with these incompatiblities as best they can through various fashions ranging from calling some obscure git and gh commands under the hood to make the GitHub PR presentation a little nicer, to a whole different UI interface for more sophisticated tools like Graphite. Another way to make the final review presentation a little cleaner is to utilize something called interdiff but that's a whole separate thing that i know less about and probably comes with its own set of learning curves. Anyways, i dont mean to get on my soapbox about this stuff and i really dont mean to oversell the newest experiments in VCS but this does seem like the kind of situation where it'd fit -- a large but anticipated feature with many discrete subcomponents and deep penetration into legacy code which we don't want breaking for a large userbase, and relatively infrequent contribution/review cycles to begin with due to the highly open source nature of the project, That being said if a few of us just got a little comfortable with it would definitely set us up for an easier time moving forward future high-impact work like the potential modalkit integration. Also this is my first time getting my hands directly on Rust's feature flag system but it's a realllly clean way to keep the risk low and probably ultimately more important than the nuances of git flows. The feature cfg marker could be a little less repetitive but i have a feeling that comes down to skill issues. |
GitHub is supposedly working on making stacked PRs a thing but i think that right now the only way to explicitly define dependencies is via Issue Blockers or of course the old fashioned manual text note edit: here's one more intriguing tool i just found called git-machete that also has some illlustrative exposition on the problem and their solutions in the README |
|
Thanks for the good explanation and links. It sounds cool. I'm just not sure I could handle it myself rn. I like the feature flag for the reasons you mentioned, it's probably the easiest understood and safest. We could totally hose all the code in the helix feature but everything else would still keep working. However, another idea I would consider is giving you (and/or maybe others) commit rights to this repo after having a chat about things to try and help make things easier. |
|
Let's see what happens next |
This is an attempt at a clean, minimal PR following from the work in #960 to provide baseline scaffolding for future work on a Helix Mode feature, mainly through a new EditMode variant that is passed to the main Reedline engine/controller.
Goals / Design choices
engine.rshelixto guard against accidental regressions to existing emacs/vi implementationsTests
Reedline.with_edit_modebuilder method provided with the newHelixEditMode variant.The code in this PR has not been tested in the context of usage in a live Reedline application, although a minimal interactive example is provided. The main purpose of this PR is to confirm feature flag isolation and some test structure for future work on the feature.
This PR should be considered ready for merge when reviewing; however if maintainers would like to see more features and demonstrated functionality beyond tests before merging, they may be added incrementally to this PR.