Add examples to ParseOptions

[brson]

And an explanation of why you would use it. Seems like it's mostly for hooking into parse warnings with log_syntax_violation. Not sure what base_url would be used for - some performance optimization? Ask on #servo.

Might want to do this as the same time as #301

[SimonSapin]

log_syntax_violation is something that is implemented because it exists in the spec but that, as far as I know, nobody has every used. I could in theory be used in an URL "validator" / conformance checker.

A base URL has nothing to do with performance. It is required to resolve relative URL references like ../foo.png. Parsing an input string like this without specifying a base URL returns an error.

ParseOptions exists to enable a builder pattern, which is a way to leave the possibility of adding more options/parameters to URL parsing without breaking the API. This in turn is a work-around for the lack of named an defaulted parameters in the language.

Since base_url is the only option really in use today and it can be more conveniently specified with the join method (some_base.join(some_string) is the same as Url::options().base_url(&some_base).parse(some_string)), I expect ParseOptions to be used very rarely.

[marti1125]

Hello @brson I would like to contribute with this =D

[dtolnay]

Thanks @marti1125! Let us know here or in the #servo IRC channel if you need any clarification.

[brson]

@marti1125 Awesome! I saw you pinged my on IRC today. Sorry I missed you. IRC is the best way to get ahold of me usually, though note I'll be on vacation next week. Please work with @dtolnay.

[marti1125]

ok =D @dtolnay what is you nick at irc ?

[dtolnay]

dtolnay

[marti1125]

at #servo ? I don't find you =(

[brson]

@marti1125 no you can find @dtolnay on #rust-libs though.

[marti1125]

@SimonSapin Can we talk at irc for fix this issue ? my nick is marti_

[dekellum]

Hi, I'm (mostly) successfully using log_syntax_violation in some crawling-related code of my own, so I can offer a (perhaps unexpected) usage example and some usage feedback which I hope you will consider.

I'm normalizing/validating URLs beyond what is strictly required by the spec, and in some cases this means wanting to promote a syntax violation warning to an Error result for filtering purposes. To work within the Fn closure limitations, I've had to use a Cell as shown below, and I'm only grabbing the last violation found. Not shown, but to grab all the violations, I'm flurting with an unsafe push into a Vec, which is awkward at best using a RefCell<Vec<String>> instead.

    /// Parse raw_url with optional base using parse options to get the
    /// last syntax violation, if any.
    fn parse_v(base: Option<&Url>, raw_url: &str)
        -> Result<(Url,Option<String>)>
    {
        let violation: Cell<Option<String>> = Cell::new(None);
        let url = {
            let vfn = & |s: &str| {
                violation.set(Some(s.to_owned()));
            };
            let options = Url::options().
                log_syntax_violation(Some(vfn)).
                base_url(base);
            options.parse(raw_url)
        }?;
        Ok((url,violation.into_inner()))
    }

So I could extract and PR doc example from this if desired? Usage feedback:

• This could be more usable for a validator as suggested, as well as help my use, if the violations were simply enumerated, not strings, and as a bonus, included an offset into the parsed original raw url str.

• Consider dropping the closure interface for an alternative parse like:

fn parse_with_violations(input: &str) -> Result<(Url,Vec<Violation>), ParseError>

Its all related, but let me know if any of this would be better in its own issue and I can create it. Thanks for your efforts!

[SimonSapin]

@dekellum You might be the first to actually use log_syntax_vidolation. In retrospect, it’s obvious that Fn rather than FnMut is rather inflexible for users. (I just tried changing it to &mut FnMut but besides being a breaking change I quickly hit many borrowck issue in the parser implementation.) As you noted, the best is probably to use RefCell.

A dedicated method goes against the idea of the ParseOptions struct with its builder pattern: log and encoding and base are multiple options available, and you might want to use any combination of them. But we don’t want a combinatorial explosion of the number of method by adding a specific method for each combination.

[dekellum]

Thanks @SimonSapin. I understand the builder pattern. I was only looking for a way around the awkward Fn usage for this one case. Thanks also for pointing out the encoding_override option. That's not in the rustdoc, presumably because its feature gated? Can that feature possibly be turned on for rustdoc?

Could you/someone offer hope that a PR would be considered, if I attempted to improve the general form and flexibility of this syntax violation feature? Not much risk of breakage, if I'm the only one using it? Right? :)

[dekellum]

The docs for ParseOptions have generally been improved, incl. for the query_encoding optional feature (#427) and for the syntax violation callback, with example usage (#433). Unless there is something actionable remaining here that I'm missing, may I suggest this be closed?

[SimonSapin]

Closing as fixed, thanks @dekellum.