Fixing minor things #8
Conversation
sgeisler
left a comment
sgeisler
left a comment
There was a problem hiding this comment.
Looks good, only left some minor nits.
|
|
||
| match iter.next() { | ||
| // If all languages were eliminated, it's an invalid word. | ||
| None => return Err(Error::UnknownWord(idx)), |
There was a problem hiding this comment.
Minor nit, please don't change it unless you deem it important yourself: technically it doesn't have to be the current word that was "invalid", it could have been any word before that, that happened to exclude some language that was actually meant. The common case is "this is an invalid word" though, so it makes a lot of sense to not overcomplicate it.
There was a problem hiding this comment.
Good point. I agree that this is the best way to return it though. Majority of cases an accidental invalid word will not by chance be valid in a random wrong language 😅
Cargo.toml
Outdated
| [features] | ||
| default = [ "std" ] | ||
| std = [ "serde/std" ] | ||
| core = [] |
There was a problem hiding this comment.
Is there any reason ever not to have core available? Does feature gating it make any sense? That feels like saying "oh, we might be missing basic arithmetic on some platforms" 😆.
There was a problem hiding this comment.
ah yeah true, core is always there, my bad, I was confused.
| /// Generate a new [Mnemonic] in the given language. | ||
| /// For the different supported word counts, see documentation on [Mnemonic]. | ||
| #[cfg(feature = "rand")] | ||
| pub fn generate_in(language: Language, word_count: usize) -> Result<Mnemonic, Error> { |
There was a problem hiding this comment.
I wonder if this should stay in here for convenience or should be thrown out for increased focus …. If SemVer is a concern: maybe it can be deprecated?
There was a problem hiding this comment.
eh I think it's ok. I like that you don't have to fiddle with rand to use this.
|
Awesome, LGTM! |
| #[cfg(feature = "rand")] | ||
| #[test] | ||
| fn test_generate() { | ||
| let _ = Mnemonic::generate(24).unwrap(); |
There was a problem hiding this comment.
Maybe these could be doctests, so that they are also improving docs (not everyone might be aware of rand_core's relation to rand)?
There was a problem hiding this comment.
Otoh, only one of these is really interesting …
There was a problem hiding this comment.
Hmm, good point :D will do
stevenroose
force-pushed
the
sebas-comments
branch
2 times, most recently
from
82b9914 to
e990720
Compare
stevenroose
force-pushed
the
sebas-comments
branch
from
e990720 to
04e139c
Compare
|
I consider that LGTM + nit as an ack 😅 |
Based on comments from @sgeisler in #6.