-
Notifications
You must be signed in to change notification settings - Fork 14
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
Setup blocklist client template #102
Conversation
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.
Some minor comments and one major comment.
I assume this closes #76, I'll link this issue with the PR. For future PRs, I recommend creating the branch from the issue itself because then you'll get both a canonical branch name and automatic linking once you open up a PR. |
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.
Nice stuff. I'd like to see the logging library updated and tokio-serde
removed (or explained why we'd keep it around) before seeing this merged.
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.
Just a minor nit.
use crate::config::Settings; | ||
use ::config::{Config, File}; |
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.
Isn't these both the same import? I think we should be explicit and go with crate::config
for both, and avoid relative imports.
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.
First one is for the config.rs at root level. Second one is from a lib.
That's what the auto-import gives and for some reason it's not happy when I don't use :: for the second one.
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!
Description
Basic setup for blocklist client using warp framework.
Closes #76
Testing information
Verified the local server but the exposed route is not integrated with a provider yet so testing will be done in subsequent commits.