Skip to content

Improve syntax violations interface and preserve Copy #433

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

Merged
merged 2 commits into from
Feb 16, 2018
Merged
Show file tree
Hide file tree
Changes from all 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
63 changes: 51 additions & 12 deletions src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ pub extern crate percent_encoding;
use encoding::EncodingOverride;
#[cfg(feature = "heapsize")] use heapsize::HeapSizeOf;
use host::HostInternal;
use parser::{Parser, Context, SchemeType, to_u32};
use parser::{Parser, Context, SchemeType, to_u32, ViolationFn};
use percent_encoding::{PATH_SEGMENT_ENCODE_SET, USERINFO_ENCODE_SET,
percent_encode, percent_decode, utf8_percent_encode};
use std::borrow::Borrow;
Expand All @@ -135,7 +135,7 @@ use std::str;
pub use origin::{Origin, OpaqueOrigin};
pub use host::{Host, HostAndPort, SocketAddrs};
pub use path_segments::PathSegmentsMut;
pub use parser::ParseError;
pub use parser::{ParseError, SyntaxViolation};
pub use slicing::Position;

mod encoding;
Expand Down Expand Up @@ -186,7 +186,7 @@ impl HeapSizeOf for Url {
pub struct ParseOptions<'a> {
base_url: Option<&'a Url>,
encoding_override: encoding::EncodingOverride,
log_syntax_violation: Option<&'a Fn(&'static str)>,
violation_fn: ViolationFn<'a>,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please store an Option<ViolationFn<'a>>and remove theNoOp` variant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I did that, it would complicate things. Firstly, I couldn't directly implement the helper methods call and call_if, given the outer type is now Option. Secondly it would complicate those methods even if I could. See the comments around call_if below.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Why not make the methods on Parser instead of ViolationFn? This would let you use Option<T>, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, for preview, in 95a7b4a I reinstate Option and move the necessary methods back to the Parser. This change would reduce the overall diff for the feature, keeping more status-quo with the parser. However it spreads around the if let Some(vfn) ... and the *_if function can't be used outside of the Parser impl. Input already uses ViolationFn::call. If the *_if variant is ever needed elsewhere this will prove a liability.

I personally still think the current PR is better without this change, but if you prefer 95a7b4a, I will include it in this branch and PR.

}

impl<'a> ParseOptions<'a> {
Expand All @@ -209,9 +209,47 @@ impl<'a> ParseOptions<'a> {
self
}

/// Call the provided function or closure on non-fatal parse errors.
/// Call the provided function or closure on non-fatal parse errors, passing
/// a static string description. This method is deprecated in favor of
/// `syntax_violation_callback` and is implemented as an adaptor for the
/// latter, passing the `SyntaxViolation` description. Only the last value
/// passed to either method will be used by a parser.
#[deprecated]
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure the method should immediately be made deprecated.

Copy link
Contributor Author

@dekellum dekellum Feb 9, 2018

Choose a reason for hiding this comment

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

Its not required for my use case. But why not give deprecation warning to those (relatively few) users of this feature to migrate to Fn(SyntaxViolation), calling v.description() themselves if that is all they want? If deprecated in the next minor release (merged), then this could be removed in some future major release, as cleanup.

pub fn log_syntax_violation(mut self, new: Option<&'a Fn(&'static str)>) -> Self {
self.log_syntax_violation = new;
self.violation_fn = match new {
Some(f) => ViolationFn::OldFn(f),
None => ViolationFn::NoOp
};
self
}

/// Call the provided function or closure for a non-fatal `SyntaxViolation`
/// when it occurs during parsing. Note that since the provided function is
/// `Fn`, the caller might need to utilize _interior mutability_, such as with
/// a `RefCell`, to collect the violations.
///
/// ## Example
/// ```
/// use std::cell::RefCell;
/// use url::{Url, SyntaxViolation};
/// # use url::ParseError;
/// # fn run() -> Result<(), url::ParseError> {
/// let violations = RefCell::new(Vec::new());
/// let url = Url::options()
/// .syntax_violation_callback(Some(&|v| violations.borrow_mut().push(v)))
/// .parse("https:////example.com")?;
/// assert_eq!(url.as_str(), "https://example.com/");
/// assert_eq!(violations.into_inner(),
/// vec!(SyntaxViolation::ExpectedDoubleSlash));
/// # Ok(())
/// # }
/// # run().unwrap();
/// ```
pub fn syntax_violation_callback(mut self, new: Option<&'a Fn(SyntaxViolation)>) -> Self {
self.violation_fn = match new {
Some(f) => ViolationFn::NewFn(f),
None => ViolationFn::NoOp
};
self
}

Expand All @@ -221,19 +259,20 @@ impl<'a> ParseOptions<'a> {
serialization: String::with_capacity(input.len()),
base_url: self.base_url,
query_encoding_override: self.encoding_override,
log_syntax_violation: self.log_syntax_violation,
violation_fn: self.violation_fn,
context: Context::UrlParser,
}.parse_url(input)
}
}

impl<'a> Debug for ParseOptions<'a> {
fn fmt(&self, f: &mut Formatter) -> fmt::Result {
write!(f, "ParseOptions {{ base_url: {:?}, encoding_override: {:?}, log_syntax_violation: ", self.base_url, self.encoding_override)?;
match self.log_syntax_violation {
Some(_) => write!(f, "Some(Fn(&'static str)) }}"),
None => write!(f, "None }}")
}
write!(f,
"ParseOptions {{ base_url: {:?}, encoding_override: {:?}, \
violation_fn: {:?} }}",
self.base_url,
self.encoding_override,
self.violation_fn)
}
}

Expand Down Expand Up @@ -363,7 +402,7 @@ impl Url {
ParseOptions {
base_url: None,
encoding_override: EncodingOverride::utf8(),
log_syntax_violation: None,
violation_fn: ViolationFn::NoOp,
}
}

Expand Down
Loading