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 Validation to Config #68

Merged
merged 2 commits into from
Jul 10, 2020
Merged

Add Validation to Config #68

merged 2 commits into from
Jul 10, 2020

Conversation

markmandel
Copy link
Contributor

We needed validation that endpoint names are unique, so implemented a validation function for config. This had a few ripple on effects:

from_reader will now call validate() to ensure that the struct is valid before completing.

validate was also left as public, since Config's can be manually created before passing them into Server.run()

We needed validation that endpoint names are unique, so implemented a
validation function for config. This had a few ripple on effects:

- impl block for Config, so made sense to pull `from_reader` into that
as a static method.
- Needed concrete Error types (impacts #67) for validation and other
potential error conditions.

`from_reader` will now call validate() to ensure that the struct is
valid before completing.

`validate` was also left as public, since Config's can be manually
created before passing them into `Server.run()`
@markmandel markmandel added the kind/feature New feature or request label Jul 8, 2020
@markmandel markmandel requested a review from iffyio July 8, 2020 00:07
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

This looks good! I think config validation is something filters will depend on a lot too down the line!

src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Outdated Show resolved Hide resolved
src/config.rs Show resolved Hide resolved
@markmandel
Copy link
Contributor Author

Updated!

Copy link
Collaborator

@iffyio iffyio left a comment

Choose a reason for hiding this comment

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

LGTM!

src/config.rs Outdated
impl Config {
/// from_reader returns a config from a given Reader
pub fn from_reader<R: io::Read>(input: R) -> Result<Config, serde_yaml::Error> {
let config: Config = serde_yaml::from_reader(input)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

rust lets us do

pub fn from_reader<R: io::Read>(input: R) -> Result<Config, serde_yaml::Error> {
    serde_yaml::from_reader(input)
}

if we wanted to avoid specify the type or rewrapping the value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, why isn't this just one line. I'll clean this up before merging.

@markmandel markmandel merged commit 5f0db6e into master Jul 10, 2020
@markmandel markmandel deleted the config/validation branch July 11, 2020 04:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes kind/feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants