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

Val err details #12

Merged
merged 19 commits into from
Jul 9, 2021
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@ let mut u = Username::new(" valid name \n\n").unwrap();
assert_eq!(u.get(), "valid name"); // now we're talking!

// This also works for mutations:
assert!(matches!(u.try_mutate(|u| *u = " ".to_owned()), Err(prae::ValidationError)));
assert!(matches!(u.try_mutate(|u| *u = " ".to_owned()), Err(prae::ValidationError<String>)));
```

Now our `Username` trims provided value automatically.
Expand Down
1 change: 1 addition & 0 deletions prae/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,6 @@ prae_macro = { version = "0.2", path = "../prae_macro" }
serde = { version = "1.0", optional = true }

[dev-dependencies]
assert_matches = "1.5"
serde = { version = "1.0", features = ["derive"] }
serde_json = "1.0"
46 changes: 34 additions & 12 deletions prae/src/core.rs
Original file line number Diff line number Diff line change
@@ -1,23 +1,45 @@
use core::hash::Hash;
use std::fmt;
use std::{error::Error, fmt};
use std::ops::{Deref, Index};

/// Default validation error. It is used for [`define!`](prae_macro::define) macro with `ensure`
/// keyword.
#[derive(PartialEq)]
pub struct ValidationError;

impl std::error::Error for ValidationError {}
/// Used for [`define!`](prae_macro::define) macro with `ensure` keyword.
#[derive(Clone, Debug)]
pub struct ValidationError<T>
where
T: fmt::Debug
{
/// The name of the type where this ValidationError originated.
guarded_type_name: &'static str,
/// The input value that caused the error.
value: T
}

impl fmt::Debug for ValidationError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
writeln!(f, "provided value is not valid")
impl<T> ValidationError<T>
where
T: fmt::Debug
{
/// Create a new ValidationError with the input value that failed.
pub fn new(guarded_type_name: &'static str, value: T) -> Self {
ValidationError { guarded_type_name, value }
}
}

impl fmt::Display for ValidationError {
impl<T> Error for ValidationError<T>
where
T: fmt::Debug
{}

impl<T> fmt::Display for ValidationError<T>
where
T: fmt::Debug
{
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
writeln!(f, "provided value is not valid")
write!(
f,
"failed to create {} from {:?}: provided value is invalid",
self.guarded_type_name,
self.value
)
}
}

Expand Down
5 changes: 3 additions & 2 deletions prae/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@
//! assert_eq!(u.get(), "valid name"); // now we're talking!
//!
//! // This also works for mutations:
//! assert!(matches!(u.try_mutate(|u| *u = " ".to_owned()), Err(prae::ValidationError)));
//! assert!(matches!(u.try_mutate(|u| *u = " ".to_owned()), Err(prae::ValidationError::<String> { .. })));
//! ```
//! Now our `Username` trims provided value automatically.
//!
//! You might noticed that `prae::ValidationError` is returned by default when our
//! You might noticed that `prae::ValidationError::<String>` is returned by default when our
//! construction/mutation fails. Altough it's convenient, there are situations when you might
//! want to return a custom error. And `prae` can help with this:
//! ```
Expand Down Expand Up @@ -79,6 +79,7 @@ pub use prae_macro::define;

#[cfg(test)]
mod test_deps {
use assert_matches as _;
use serde as _;
use serde_json as _;
}
Comment on lines 82 to 87
Copy link
Owner

Choose a reason for hiding this comment

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

Btw, why is this necessary?

Copy link
Owner

Choose a reason for hiding this comment

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

I mean the whole test_deps business

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added the lint to warn about unused crate dependencies.

#![warn(unused_crate_dependencies)]

But it give's false positives when buliding the separate integration test binaries.

❯ cargo test
   Compiling prae_macro v0.2.1 (/home/brianh/Projects/third_party/prae/prae_macro)
   Compiling prae v0.4.5 (/home/brianh/Projects/third_party/prae/prae)
warning: external crate `assert_matches` unused in `prae`: remove the dependency or add `use assert_matches as _;`
   |
note: the lint level is defined here
  --> prae/src/lib.rs:71:9
   |
71 | #![warn(unused_crate_dependencies)]
   |         ^^^^^^^^^^^^^^^^^^^^^^^^^
   = help: remove unnecessary dependency `assert_matches`

warning: external crate `serde` unused in `prae`: remove the dependency or add `use serde as _;`
  |
  = help: remove unnecessary dependency `serde`

warning: external crate `serde_json` unused in `prae`: remove the dependency or add `use serde_json as _;`
  |
  = help: remove unnecessary dependency `serde_json`

warning: 3 warnings emitted

    Finished test [unoptimized + debuginfo] target(s) in 0.73s
     Running unittests (target/debug/deps/prae-dc495198ddde61f3)

Copy link
Owner

Choose a reason for hiding this comment

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

Oh, now I get it. I'll add a comment referencing the related issue: rust-lang/rust#57274

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice 😉

15 changes: 10 additions & 5 deletions prae/tests/adjust_ensure.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use assert_matches::assert_matches;

use prae;

prae::define! {
Expand All @@ -8,7 +10,10 @@ prae::define! {

#[test]
fn construction_fails_for_invalid_data() {
assert_eq!(Username::new(" ").unwrap_err(), prae::ValidationError);
assert_matches!(
Username::new(" ").unwrap_err(),
prae::ValidationError::<String> { .. }
)
}

#[test]
Expand All @@ -20,10 +25,10 @@ fn construction_succeeds_for_valid_data() {
#[test]
fn mutation_fails_for_invalid_data() {
let mut un = Username::new("user").unwrap();
assert_eq!(
assert_matches!(
un.try_mutate(|u| *u = " ".to_owned()).unwrap_err(),
prae::ValidationError
);
prae::ValidationError::<String> { .. }
)
}

#[test]
Expand All @@ -38,4 +43,4 @@ fn mutation_succeeds_for_valid_data() {
let mut un = Username::new("user").unwrap();
assert!(un.try_mutate(|u| *u = " new user ".to_owned()).is_ok());
assert_eq!(un.get(), "new user");
}
}
10 changes: 6 additions & 4 deletions prae/tests/adjust_validate.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use assert_matches::assert_matches;

use prae;

#[derive(Debug, PartialEq)]
#[derive(Debug)]
pub struct UsernameError;

prae::define! {
Expand All @@ -17,7 +19,7 @@ prae::define! {

#[test]
fn construction_fails_for_invalid_data() {
assert_eq!(Username::new(" ").unwrap_err(), UsernameError {});
assert_matches!(Username::new(" ").unwrap_err(), UsernameError {});
}

#[test]
Expand All @@ -29,7 +31,7 @@ fn construction_succeeds_for_valid_data() {
#[test]
fn mutation_fails_for_invalid_data() {
let mut un = Username::new("user").unwrap();
assert_eq!(
assert_matches!(
un.try_mutate(|u| *u = " ".to_owned()).unwrap_err(),
UsernameError {}
);
Expand All @@ -47,4 +49,4 @@ fn mutation_succeeds_for_valid_data() {
let mut un = Username::new("user").unwrap();
assert!(un.try_mutate(|u| *u = " new user ".to_owned()).is_ok());
assert_eq!(un.get(), "new user");
}
}
22 changes: 17 additions & 5 deletions prae/tests/ensure.rs
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
use assert_matches::assert_matches;

use prae;

prae::define! {
Expand All @@ -7,7 +9,17 @@ prae::define! {

#[test]
fn construction_fails_for_invalid_data() {
assert_eq!(Username::new("").unwrap_err(), prae::ValidationError);
assert_matches!(
Username::new("").unwrap_err(),
prae::ValidationError::<String> { .. }
)
}

#[test]
fn error_formats_correctly() {
let error = Username::new("").unwrap_err();
let message = format!("{}", error);
assert_eq!(message, "failed to create Username from \"\": provided value is invalid");
}

#[test]
Expand All @@ -19,10 +31,10 @@ fn construction_succeeds_for_valid_data() {
#[test]
fn mutation_fails_for_invalid_data() {
let mut un = Username::new("user").unwrap();
assert_eq!(
assert_matches!(
un.try_mutate(|u| *u = "".to_owned()).unwrap_err(),
prae::ValidationError
);
prae::ValidationError::<String> { .. }
)
}

#[test]
Expand All @@ -37,4 +49,4 @@ fn mutation_succeeds_for_valid_data() {
let mut un = Username::new("user").unwrap();
assert!(un.try_mutate(|u| *u = " new user ".to_owned()).is_ok());
assert_eq!(un.get(), " new user ");
}
}
10 changes: 6 additions & 4 deletions prae/tests/validate.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
use assert_matches::assert_matches;

use prae;

#[derive(Debug, PartialEq)]
#[derive(Debug)]
pub struct UsernameError;

prae::define! {
Expand All @@ -16,7 +18,7 @@ prae::define! {

#[test]
fn construction_fails_for_invalid_data() {
assert_eq!(Username::new("").unwrap_err(), UsernameError {});
assert_matches!(Username::new("").unwrap_err(), UsernameError {});
}

#[test]
Expand All @@ -28,7 +30,7 @@ fn construction_succeeds_for_valid_data() {
#[test]
fn mutation_fails_for_invalid_data() {
let mut un = Username::new("user").unwrap();
assert_eq!(
assert_matches!(
un.try_mutate(|u| *u = "".to_owned()).unwrap_err(),
UsernameError {}
);
Expand All @@ -46,4 +48,4 @@ fn mutation_succeeds_for_valid_data() {
let mut un = Username::new("user").unwrap();
assert!(un.try_mutate(|u| *u = " new user ".to_owned()).is_ok());
assert_eq!(un.get(), " new user ");
}
}
6 changes: 3 additions & 3 deletions prae_macro/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,9 +34,9 @@ pub fn define(input: TokenStream) -> TokenStream {

let validate_fn = match &guard {
GuardClosure::Ensure(EnsureClosure(closure)) => quote! {
fn validate(v: &Self::Target) -> Option<prae::ValidationError> {
fn validate(v: &Self::Target) -> Option<prae::ValidationError::<#ty>> {
let f: fn(&Self::Target) -> bool = #closure;
if f(v) { None } else { Some(prae::ValidationError) }
if f(v) { None } else { Some(prae::ValidationError::<#ty>::new(stringify!(#ident), v.clone()) ) }
Copy link
Owner

Choose a reason for hiding this comment

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

What if the type is not clone? In this case the user will use Guarded::mutate instead of Guarded::try_mutate, but they both use validate

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, exactly. The following code works on HEAD but not on this branch:

#[derive(Debug)]
struct User {
    name: String,
}

prae::define! {
    ValidUser: User
    ensure |u: &User| !u.name.is_empty()
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's either clone, copy or have the error manage reference lifetimes. I would like to avoid having lifetimes as that constraint will leak into the users code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we can add a nicer message though, instead of a compile error emanating from the proc macro?

Copy link
Owner

Choose a reason for hiding this comment

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

I guess it's also can't be done with the current code. Introducing ConstructionError<E> and MutationError<E> should solve the issue, since we'll be able to insert the value: T inside Guarded::new and Guarded::try_mutate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see a way around cloning, as we have to clone at some point so the error can store the value, or.... you convert the value to a string which requires T: Debug or T: DIsplay.

Requiring Clone on a type is a reasonable expectation given that it's trivial to implement and it's the exception that you see types that are not clone-able.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The latest commit removes the value.

Copy link
Owner

@teenjuna teenjuna Jul 9, 2021

Choose a reason for hiding this comment

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

Making it T: Debug and turning it into string is a reasonable workaround! But expecting the value to be always Clone is a no go, since the library must be available for as many use cases as possible. Sometimes making your value !Clone is a design choice, and we must tolerate this.

Do you want to add str_value: String or should I do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I can do it. I'll update the non_clone test too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ValidationError::value is reinstated as a String, changes are pushed.

}
},
GuardClosure::Validate(ValidateClosure(closure, err_ty)) => quote! {
Expand All @@ -48,7 +48,7 @@ pub fn define(input: TokenStream) -> TokenStream {
};

let err_ty = match &guard {
GuardClosure::Ensure(_) => quote!(prae::ValidationError),
GuardClosure::Ensure(_) => quote!(prae::ValidationError::<#ty>),
GuardClosure::Validate(ValidateClosure(_, err_ty)) => quote!(#err_ty),
};

Expand Down