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

Add set_name and clear_attributes functions to BytesStart #149

Merged
merged 1 commit into from
Feb 19, 2019

Conversation

meven
Copy link
Contributor

@meven meven commented Feb 15, 2019

Fixes #148

@jonhoo
Copy link

jonhoo commented Feb 15, 2019

Those fns should be pub fn, that's why you're getting the warnings :)

@meven
Copy link
Contributor Author

meven commented Feb 16, 2019

Those fns should be pub fn, that's why you're getting the warnings :)

Thanks ;) that's been taking care of.

@jonhoo
Copy link

jonhoo commented Feb 16, 2019

@tafia looks like the Travis failure is just rustfmt-preview being unavailable for nightly (the component is now just called rustfmt).

Copy link
Owner

@tafia tafia left a comment

Choose a reason for hiding this comment

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

Looks great.
Could you just amend the comment to avoid confusion?

src/events/mod.rs Show resolved Hide resolved
@tafia
Copy link
Owner

tafia commented Feb 19, 2019

Thanks for the PR and sorry for the late response.
Can you please just modify the comment on set_name?
I'll push changes to travis in the meantime.

@meven meven force-pushed the new-bytesStart-methods branch 2 times, most recently from d3cf373 to 85d74b0 Compare February 19, 2019 08:02
@meven
Copy link
Contributor Author

meven commented Feb 19, 2019

Last change made me think that perhaps set_names should not edit the attributes and keep them.
It would end up as :

    /// Edit the name of the BytesStart in-place
    pub fn set_name(&mut self, name: &[u8]) -> &mut BytesStart<'a> {
        {
            let bytes = self.buf.to_mut();
            bytes.splice(..self.name_len, name.to_vec());
        }
        self.name_len = name.len();
        self
    }

@tafia
Copy link
Owner

tafia commented Feb 19, 2019

Sounds better indeed!

@meven
Copy link
Contributor Author

meven commented Feb 19, 2019

Sounds better indeed!

Done, I feel like this can be merged

@tafia
Copy link
Owner

tafia commented Feb 19, 2019

Thanks!

@tafia tafia merged commit 1eff4fa into tafia:master Feb 19, 2019
@meven meven deleted the new-bytesStart-methods branch February 19, 2019 12:08
pub fn set_name(&mut self, name: &[u8]) -> &mut BytesStart<'a> {
{
let bytes = self.buf.to_mut();
bytes.splice(..self.name_len, name.to_vec());
Copy link

Choose a reason for hiding this comment

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

@meven I don't think the to_vec call here is needed. It'd be good to avoid the extra allocation!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not find a better way I would expect the same but I ran into

error[E0271]: type mismatch resolving `<&[u8] as std::iter::IntoIterator>::Item == u8`
   --> src/events/mod.rs:178:19
    |
178 |             bytes.splice(..self.name_len, name);
    |                   ^^^^^^ expected reference, found u8
    |
    = note: expected type `&u8`
               found type `u8`

It seems I still need some conversion, name &[u8] implements IntoIterator<u8> but I need an IntoIterator<&u8>. Maybe There is some simple trick I am missing.

Copy link

Choose a reason for hiding this comment

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

Ah, I've run into this in the past. It's a little silly. Basically what you want is name.iter().cloned() or name.iter().map(|&b| b) to tell the compiler that you want to utilize the fact that u8 is Copy.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, will make another PR

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