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 push that fails if upper bound is violated #9

Open
greenhat opened this issue Oct 10, 2021 · 5 comments
Open

Add push that fails if upper bound is violated #9

greenhat opened this issue Oct 10, 2021 · 5 comments

Comments

@greenhat
Copy link
Member

from #8 by @SethDusek:

This will be useful for using BoundedVec in sigma-rust. Also, I was wondering, what are your thoughts on adding a try_push function to BoundedVec? It'd make it a bit easier to use, it could return an Option or Result if there's no more room in the vector.

@greenhat
Copy link
Member Author

This is an excellent question! Actually, I wonder if we should make all existing add/remove methods (e.g. push, pop, append, etc.) failable. I mean we have lower and upper bounds and we should error out in case we're going out of them. That'd make BoundVec API not so similar to Vec anymore, but I'm not sure it's worth sticking to it that much. What do you think?

@SethDusek
Copy link
Collaborator

SethDusek commented Oct 11, 2021

There's some code in the WASM bindings for sigma-rust where I felt it would be useful, namely, Tokens, which currently wraps a Vec, but I plan on changing it to BoundedVec.

impl Tokens {
    /// Adds an elements to the collection
    pub fn add(&mut self, elem: &Token) {
        self.0.push(elem.clone());
    }
}

Without a try_add, this becomes a bit trickier since you need to take the BoundedVec, consume it, and create a new one. An additional suggestion is to also make Tokens::add fallable, thus this could become:

pub fn add(&mut self, elem: &Token) -> Result<(), OutOfBounds) {
    if let Some(tokens) = self.0 {
         tokens.try_add(elem.clone())?; // BoundedVec::try_add
    }
    else {
         self.0 = Some(BoundedVec::from([elem.clone()]));
    }
    Ok(())
}

@greenhat
Copy link
Member Author

I totally understand the need for this. What I'm thinking is to name it not try_add but push like in Vec with a signature fn push(&mut self, elem: T) -> Result<(), OutOfBounds> {...}. Because any addition op is failable. Unless you think it might mess people up because everyone is got used to Vec::push is not failable. This is the only downside I found so far.

@SethDusek
Copy link
Collaborator

That seems pretty reasonable yea. By default rustc warns about unused Results anyways, so people would adapt quickly

@greenhat greenhat changed the title Add try_push that fails if upper bound is violated Add push that fails if upper bound is violated Oct 11, 2021
@greenhat
Copy link
Member Author

greenhat commented Oct 11, 2021

Good point! I can see that existing familiarity with Vec::push might even help.

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

No branches or pull requests

2 participants