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

Allow getting the buffer handle and make Buffer copy #80

Closed
wants to merge 2 commits into from

Conversation

oberblastmeister
Copy link
Contributor

Getting the internal handle is useful when nxim_oxi hasn't made bindings to something yet. I also don't see why Buffer shouldn't be Copy, as it's just an integer. I also don't see how the internals of Buffer could be changed to something !Copy in the future. I think the other types like Window should be made Copy too.

@oberblastmeister
Copy link
Contributor Author

@noib3 Is this okay?

@noib3
Copy link
Owner

noib3 commented Jan 28, 2023

@noib3 Is this okay?

@oberblastmeister sorry for keeping it open so long, I'll make sure to review this this weekend.

@@ -88,6 +88,10 @@ impl Pushable for Buffer {
}

impl Buffer {
pub fn handle(&self) -> BufHandle {
Copy link
Owner

Choose a reason for hiding this comment

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

It's missing a doc comment explaining what it returns.

@@ -27,7 +27,7 @@ use crate::LUA_INTERNAL_CALL;
use crate::{Error, Result};

/// A wrapper around a Neovim buffer handle.
#[derive(Clone, Eq, PartialEq, Hash, Serialize, Deserialize)]
#[derive(Copy, Clone, Eq, PartialEq, Hash, Serialize, Deserialize)]
Copy link
Owner

Choose a reason for hiding this comment

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

I'm not against making the handle wrapper types (i.e. Buffer, Window and TabPage) Copy (in fact at one point they were), but I'd change them all in the same commit.

@noib3 noib3 force-pushed the master branch 3 times, most recently from 0278204 to 6f23cf3 Compare April 1, 2023 17:22
@noib3 noib3 force-pushed the master branch 2 times, most recently from 9e63751 to 3f75ed9 Compare April 5, 2023 22:54
@noib3 noib3 force-pushed the main branch 3 times, most recently from e70b241 to 6124b57 Compare October 11, 2023 13:17
@noib3 noib3 force-pushed the main branch 2 times, most recently from a1882fd to c1a89ca Compare December 15, 2023 12:32
@noib3
Copy link
Owner

noib3 commented Jun 14, 2024

Closed by #176, which added a fn handle(&self) -> i32 method on Buffer, Window and TabPage.

Neither of those is Copy atm, but I think if we were to make them we should also probably change all the methods from taking &self and &mut self to taking self, which is a breaking change.

@noib3 noib3 closed this Jun 14, 2024
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

2 participants