-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Clarify Context
#6056
Clarify Context
#6056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
compositor::Context
is actually the more idiomatic naming convention, so this is not a good change even on the face of it (e.g. imagine if std::io::Result
was std::io::IoResult
). But even so, we shouldn't be doing these large scale renames just for the sake of it. They serve no purpose but your own personal preference, while breaking a large number of open PRs. There should be a strong justification for these kinds of changes.
I agree, the namespacing was intentional here. Not all types in a codebase need to have a unique name. Similarly |
That would be good and all were it not for the fact that Also, signature help never shows the fully qualified module path, only Here are just two examples where it is quite ambiguous what type helix/helix-term/src/ui/editor.rs Lines 1234 to 1248 in 44729fb
Answer`cx` on line 1236 is `command::Context` and `cx` on line 1244 is `compoitor::Context`.helix/helix-term/src/commands.rs Lines 5305 to 5317 in 44729fb
Answer`cx` on line 5305 is `commands::Context` and `cx` on line 5317 is `compositor::Context`.I would argue that there are about ~1600 or so more examples (each time In a perfect world, I would have preferred Sure, the other PRs of mine will break some existing PRs. But I really do believe there should be an increased effort in keeping the code base readable and in turn maintainable. Not merging refactorings because "it will break other PRs" is a sure fire way to amassing exponential technical debt. And frankly, that is in my opinion where Helix is now in many of its components. I really don't want to bash on you guys, the work you've done truly is awesome. And it's not like I want to make your life harder. Quite the opposite, being open to code readability improvements will in the end make future reviews much easier. |
A lot of this is subjective and we could of course argue about variable naming all day. I preferred the current version of the code which is why I named the variables that way in the first place. I agree with a lot of the functional cleanups you've made in your keymap PR but as we've previously stated renaming and moving code around shouldn't be done unless there's a clear benefit beyond just personal preference. The Context renaming here can be argued and it might fit the bar but a lot of these do not (bbfeafd) and only make actual changes harder to review. I'd gladly merge #6061 today if it didn't also hold other changes for example but now it's going to be stuck until we can clarify the remaining commits in the branch. |
Here's an example of the lengths we're going to in order to avoid breaking lots of open work: #5581 The refactor there is a lot more crucial (it unblocks new functionality) but we're ensuring there will be close to no conflicts to resolve. For each PR with break we risk the author abandoning the work and having to adopt the PRs ourselves. |
Ok, so instead of going back and forth with "this might not fit the bar because it might break a lot of PRs", I've now thrown together a bash script that checks if local branch conflicts with open PRs that are mergeable with master, and pass all CI checks.
Here's the script if you're interested in trying it out: https://github.com/gibbz00/conflict |
Hm, that's funny, because I ran your script locally and after merging this PR into master, there are 32 conflicting PRs.
It's also likely under counting because it's not counting PRs that have existing conflicts but would also conflict with this change after they were fixed, nor is it counting PRs that would conflict after their CI checks pass. But you're also not listening.
Breaking PRs is inevitable—that is just going to happen when more than one person is working on the same code base. But the thing you're not getting is that we value all contributions, and the harder we make it for people to contribute, the less likely they are to do so. Every time we make someone rebase and fix conflicts, we risk frustrating them and losing their PR. So we must take care to avoid needless breakage that does not serve a good purpose. This kind of "rename a type to suit my preferences" PR not only adds very little value to people other than yourself, but takes a shotgun to other people's contributions that we must ask them to repair. And it also makes it especially difficult to review PRs with actual value in them when this kind of change is thrown in. |
Oopsie, wrong branch on my end 😅 But yeah also got 32 conflicting PRs now. #6061 also got 32, which I presume all came from this one. So I've now closed #6061 and opened up a new one with cherry-picked commits only relevant to I'll refrain from countering the other points you made, as I don't think anything constructive will come out of it. I will keep this PR open and leave it up to @archseer to decide if he wants it closed. If he does; we just move on. To which I would be fine with now that I've had the opportunity to express my intent behind this PR. |
Hello! As a new contributor who recently has been learning the code base I have some fresh, um, context here. Please take this with a grain of salt as I've only been poking at the code a few weeks. I do think the "what context is this" problem is real for understanding the codebase in the
I find myself wondering why many functions aren't just accepting a combination of Editor, View and Document borrows. This also helps with the borrow checker not always getting field-level borrows correctly (especially true through function boundaries). Would a reduction in proliferation of passing around Context objects go further to reduce cognitive load than trying to name the Context objects better or make sure they are relatively qualified (e.g., Obviously a giant PR changing the signature of 1,000 functions isn't going to help anyone either, but in my experience just documenting a change in direction alongside some motivation and historical context can elicit a lot of momentum (e.g., via ADRs). |
The I think passing all these arguments around manually would be much more unreadable. Someone looking at a command just needs to know: this is the context the command is executed in, which is the commands way to interact with the rest of the codebase and then only needs to care about the parts of the context a command uses (usually the editor). This is also the only real sustainable way to pass arguments to commands. Imagine if every command received all these arguments explicitly and we decided to remove or change one of these fields. If they are all function parameters you would need to change all commands (and there are a LOT of commands). This would not only produce a huge diff in every change but also cause a huge number of merge conflicts and (arguably even worse) would break any PR that adds a new command (since the signature is now inccorect) without even showing up as a conflict (or causing a CI failure until the PR is merged or rebased) |
I agree that having a Context to keep a stable signature for command handlers makes sense, but I'm not sure it's the only way to solve such a problem. I'm also not suggesting it's a bad way to solve such a problem. I'm academically interested in considering alternatives. Looking back through the code, it does seem the prevailing use of Context is in the entrypoint of command handlers. I may be mis-remembering it being more pervasive than that. Many of the factored out functionality does operate on at least an Editor or even small units of data. From a testing perspective it does force all command testing up into integration tests, since replacing a Context is a challenging endeavor. I don't really know a better pattern there than using trait-based generics for the needed functionality.
My suggestion was more along the lines of, "Imagine if every command received only the arguments it performed operations on or with". As it stands the Context is a bit of a god object that is sliced and diced to find what each command needs. Effectively, it can only grow over time. |
This would be a maintenance nightmare. This effectively would mean there is no standard API for every command, and every command execution and handling would have to be special cased. By comparison, having extraneous data in a context object for some commands is a small price to pay, if indeed there is any practical cost at all.
Do you have any other suggestions? |
I want to clarify that I'm not (yet) an expert in Rust just interested in having a discussion and shared the OPs challenge in acclimating to the code base in this one area as well. I'm concerned I'm offending people by bringing the discussion up at all, and that's certainly not my intention. Working on Helix is really nice compared to many projects of this scale. For better or worse, my personality is to always push on the structures of an architecture and see where it bends and breaks in the hopes of making meaningful improvements.
Yeah I'm not suggesting it would be an improvement to hard-wire up a billion functions. Macros might be one option, or classifying functions into different buckets based on their needs (e.g., a function type that operates on the current document and view, versus one that operates on the editor as a whole).
The one example I have that came to mind is how bevy implements systems as plain functions for the ECS model. It uses generics that specify what types of entities will be passed to the handler function so that you end up with a strongly typed interface with copies of or references to the entities you wish to operate on. |
This is a net benefit from my view. "functional core imperative shell" (also the reason selections and transactions are immutable). Commands are composed of
Historically (~2+ years ago): I started with a simpler context that used only these fields (passing in a current doc and view) and the trouble is that some commands end up using multiple views or multiple documents. You can't pass both a mutable Editor borrow and a Document borrow (that lives on that Editor) at the same time. I've gone through a few iterations here and thought about the API. I've also considered returning
It's much more tenable to have a single, versatile command signature that doesn't make any assumptions about what the command is trying to do.
It really hasn't though. It has a reference to the editor, the current input count and the current register. The rest is just plumbing to allow you to schedule async jobs. |
Great context, I really appreciate you writing this up.
You know it's funny, the first things I wanted to poke at were commands and I found writing tests difficult* because I couldn't reasonably pass state into a command and inspect it's output (as you say below, by getting an (*I have not yet taken the time to learn the integration testing suite, my bad.)
Fair enough! |
Summary:
Context
->CompositorContext
Context
->CommandContext
Driving me nuts when trying to understand code with
cx
/ctx
/cxt
when both commands::Context and compositor::Context are in scope.