-
Notifications
You must be signed in to change notification settings - Fork 65
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
general cleanup and maintenance #362
Conversation
changed `StatusCode` from `i31` to `u16`. refactored logic expanded documentation added TODOs made some functions more generic (to improve API w/o causing breaking changes)
1f2f3d8
to
2277291
Compare
Whao! Thanks for taking the time to do this. Let's do this MR as usual, but going forward, it'll be much easier for me if we have smaller-sized changes! I know it's hard sometimes, especially when we combine everything from a working session into a single MR, but it makes it easier for me as I don't have all the time in the world (that way we can maintain shipping velocity!). I hope you understand :) 🤗 |
sure, I can have this as base then do subsequent PR's to address the TOOD's and whatnot |
I forgot why I hadn't merged this yet, it's been a while... |
Okay I think it's good now, gonna do one final review before I merge though |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
Ok(s) => Box::leak(s.into_boxed_str()).as_bytes(), | ||
Err(_) => panic!("No SECRET_KEY environment variable set!"), | ||
}, | ||
secret: std::env::var("SECRET_KEY").map_or_else(|_| panic!("No SECRET_KEY environment variable set!"), |s| Box::leak(s.into_boxed_str()).as_bytes()), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we might be able to just use a .map(...).expect(...)
since we panic anyway
/// | ||
/// panis if the `to` argument is not a valid email address | ||
/// | ||
/// TODO: wouldn't it be better to instead require the `to` argument be some wrapper around a string that is always a valid email address? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
answer: yes it would, but that's something to do in a different PR
StatusCode
fromi31
tou16
.this is just a start, there's a lot of TODOs I still need to tackle as well