Skip to content
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

Identifiers are now real types #292

Merged
merged 3 commits into from
May 15, 2017
Merged

Identifiers are now real types #292

merged 3 commits into from
May 15, 2017

Conversation

cmyr
Copy link
Member

@cmyr cmyr commented May 15, 2017

This patch makes better use of the type system to distinguish between BufferIdentifier and ViewIdentifier.

There were a few spots in the plugin code where this was poorly distinguished, and this change makes me a bit more considered about those places. It also will make it easier to use some value type as an identifier in the future.

Copy link
Collaborator

@trishume trishume left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good other than one piece

.editor_for_view(&view_id2) {
ed.plugin_started(&view_id2, &key.1);
running = true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This close paren is mis-indented.

But more importantly, why did you change this from how it was before to add the running variable? Since running only becomes true in the if block, why not remove the running variable and put inner.running.insert(key, plugin_ref) inside the if block.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm, I was doing it this way because inner.running.insert is a mutable borrow, and we have an immutable borrow out when we take editor_for_view. It's possible I'm missing something. Just moving it into the if block doesn't compile.

My editor wants to align that closing brace with the second line of the opening expression. I don't really have an opinion here, will change, but it might get reverted next time I fix whitespace 😐.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darn borrow checker. Having to do dumb things like that for no good reason when using mutable borrows is one of my least favourite things about Rust.

@cmyr cmyr merged commit 0604b1d into xi-editor:master May 15, 2017
@cmyr cmyr deleted the ident-types branch May 15, 2017 18:48
lord pushed a commit to lord/xi-editor that referenced this pull request Oct 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants