semver example: find the latest version satsifying given range

[anna-liao]

fixes #285 r? @budziq

[ludwigpacifici]

Let's use error_chain and replace unwrap() by ?. If the reader copy paste the snippet and modify the version with a wrong value, the program will exit with a message error more useful than a panic.

[ludwigpacifici]

Hello @anna-liao

Thank you for your PR!

Here are some suggestions to help you improve your semver example:

• do not forget to update intro.md file so that it also appears in the table of contents
• @budziq, should the example be enriched by adding pre-release tag into the versions? If yes, the crate as a specific behaviour that is not obvious and might be worth to highlight (crate code).

[ludwigpacifici]

Let's try to make this for loop more Rust idiomatic by using iterators. For example:

1. filter_map to convert valid vset string versions to semver::Version
2. filter versions which does not match your VersionReq
3. and use max to get the latest version from the range

[anna-liao]

@ludwigpacifici Thanks for the review! I have updated my PR, though have not added the pre-release tag yet, pending response from @budziq

[ludwigpacifici]

You can make your example more concise by removing mod errors scope.

[budziq]

this whole mod can be deleted.

[ludwigpacifici]

You can make your example more concise by removing vset and go ["0.9.0", "1.0.0", "1.0.1"].iter()...

[budziq]

agreed 👍

[budziq]

also we can use slightly longer variable names to improve readabaility

[ludwigpacifici]

I would give more details in the description. For example, explain the range is obtained via the matches and the max.

[budziq]

Agreed. I'd remove the "an array" and "unparsed". I'd also add the hyperlinks to key identifiers like @ludwigpacifici suggests, Imho these would be VersionReq::parse, VersionReq::matches and optionally Version::parse.

Additionally "latest" is quite imprecise, I'd suggest rewriting it as 'satisfying the given range "<= 1.0.0"' .

[ludwigpacifici]

Thanks @anna-liao. I added some more comments, but I would suggest you to wait for @budziq feedback and then proceed to the changes. I just saw he mentioned another solution (with sets) to implement it. Let's see which is more suitable for this example.

[budziq]

@anna-liao Thanks. Few suggestions.

[budziq]

@anna-liao Thanks. Few suggestions.

[budziq]

this whole mod can be deleted.

[budziq]

please checkout our note about error handling.

The error boilerplate (error definition and error-chain import) should be hidden with '#'

[budziq]

SemVerError is not actually used in this example

[budziq]

you can drop the chain_err if you add semver::ReqParseError to the foreign_links

[budziq]

agreed 👍

[budziq]

lets hide the error boilerplate

[budziq]

lets import both Version and VersionReq with a single statement

[budziq]

lets run rustfmt against the example

[budziq]

the & is unnecessary here

[budziq]

Agreed. I'd remove the "an array" and "unparsed". I'd also add the hyperlinks to key identifiers like @ludwigpacifici suggests, Imho these would be VersionReq::parse, VersionReq::matches and optionally Version::parse.

Additionally "latest" is quite imprecise, I'd suggest rewriting it as 'satisfying the given range "<= 1.0.0"' .

[anna-liao]

Thanks for the reviews, @ludwigpacifici and @budziq ! Really appreciate you both taking the time to go over my PR with a fine tooth comb.

I am having an issue while revising the PR:
When I click on the link I added in intro.md after running mdbook serve -o, it jumps to the wrong example. Specifically, it jumps to the ex-semver-command example. I am pretty sure I wrote the link format correctly. I am running this on Ubuntu 16.04

[ludwigpacifici]

To ease readability, links are placed at the bottom of the file in alphabetic order.

No need for double space after a dot. Maybe I am too picky? :-)

• "requirement. Filter"
• "matches). Return"

[ludwigpacifici]

I am having an issue while revising the PR:
When I click on the link I added in intro.md after running mdbook serve -o, it jumps to the wrong example. Specifically, it jumps to the ex-semver-command example. I am pretty sure I wrote the link format correctly. I am running this on Ubuntu 16.04

Nice catch @anna-liao! I do not see something obvious in your PR that can explain that. Also, I can reproduce your issue with Firefox with different links:

• https://rust-lang-nursery.github.io/rust-cookbook/net.html#ex-extract-links-webpage
• https://rust-lang-nursery.github.io/rust-cookbook/basics.html#ex-measure-elapsed-time

With Chrome it is fine. Not sure if the issue is cookbook or mdbook related.

@budziq, in case you missed it, did you have time to look at the below (from top of this thread)? :-)

@budziq, should the example be enriched by adding pre-release tag into the versions? If yes, the crate as a specific behaviour that is not obvious and might be worth to highlight (crate code).

[budziq]

With Chrome it is fine. Not sure if the issue is cookbook or mdbook related.

I can reproduce it only on Firefox 57 which due to stylo is more prone to incomplete style flashes. What we experience here is firefox pointing to correct page location but then a highlight.js and mdbooks custom js kicks-in to style the code snippets and hide the lines starting with '#'. The links you are mentioning are in places particularly heavy with '#'. Hiding the '#' lines causes the page to get shorter but firefox viewport does not move anymore. I'm not versed in cross-browser compatible html+js+css so I cannot say if that this is a problem with either firefox or mdbook but it is certainly not a problem with the cookbook. I guess that we should start with posting the issue to mdbook bugtracker. Anyone care to do that? :)

should the example be enriched by adding pre-release tag into the versions?

I'm not particularly sure if we should focus on teaching semver intricacies as the mentioned behavior is just part of the standard but if we were able to present it in a compact form then I'm game! If we were to go that way I'd suggest to extract the comparing/searching logic into a function and call it two times (once on a dataset that we already have and once on a version with prerelease tag)

[budziq]

Minor suggestions below 👍

[budziq]

Minor suggestions below 👍

[budziq]

I'd probably refrain from describing all of the code step by step (especially the last sentence might be omitted).

[anna-liao]

@budziq @ludwigpacifici I updated the description so that it is concise.

Regarding an example with the pre-release tags, I am working on that in a separate branch and will submit as a PR when it is working.

[budziq]

I'd probably try to keep the description more compact. Haw about something along these lines?:

Given a list of version &strs, finds the latest [semver::Version] that satisfying given [semver::VersionReq] "<= 1.0.0" using [VersionReq::matches].

[ludwigpacifici]

Thanks @anna-liao for raising an mdBook issue.

I'm not particularly sure if we should focus on teaching semver intricacies as the mentioned behavior is just part of the standard but if we were able to present it in a compact form then I'm game! If we were to go that way I'd suggest to extract the comparing/searching logic into a function and call it two times (once on a dataset that we already have and once on a version with prerelease tag)

I raised this idea because I tried this example with these inputs ["0.9.0", "1.0.1", "0.9.0-abc"] and I was surprised to see 0.9.0 as a result. I though it was bug. In the end I found out, it was by definition. I agree, it's a bit border line between cookbook and semver specifications. Refactor the code in a function and call it on two different examples is a good approach and not too intrusive.

[anna-liao]

Added new version with pre-release example. r? @ludwigpacifici @budziq

[ludwigpacifici]

You can remove this comment

[ludwigpacifici]

Since the expected result can be surprising, I wonder if a comment should be added like: "Shows Semver precedence for pre-release tags"

[budziq]

few more suggestions

[budziq]

few more suggestions

[budziq]

If you go with signature such as this one:

fn find_max_matching_version<'a, I>(
    version_req_str: &str,
    iterable: I,
) -> Result<Option<Version>>
where
    I: IntoIterator<Item = &'a str>,
{

Then you will be able to omit both into_iter() calls below and

also rustfmt allows us to put whole function declaration in a single line so lets do it for readability.

[anna-liao]

@budziq When I run the code with this signature, I get this error:

error[E0599]: no method named `filter_map` found for type `I` in the current scope
  --> src/main.rs:33:10
   |
33 |         .filter_map(|s| Version::parse(s).ok())
   |          ^^^^^^^^^^
   |
   = note: the method `filter_map` exists but the following trait bounds were not satisfied:
           `&mut I : std::iter::Iterator`
   = help: items from traits can only be used if the trait is implemented and in scope
   = note: the following trait defines an item `filter_map`, perhaps you need to implement it:
           candidate #1: `std::iter::Iterator`

I couldn't figure out how to resolve this compiler error. Do I need to implement filter_map for I?

[ludwigpacifici]

@anna-liao , have you tried to remove the two below into_iter() calls (in the asserts) and go iterable.into_iter().filter_map(... in find_max_matching_version function?

[budziq]

Do I need to implement filter_map for I?

Nope. Actually the error message could be a lot better here.

Let me try to explain what is going on here. IntoIterator is a trait bound here making the find_max_matching_version function generic over type implementing the trait. So it will accept now things like Vec, HashSet (and almost any other container) or any Iterator as long as their Item is &str.

But while all these types are accepted as the parameter, they are not an iterator yet (which itself implement filter_map). You will have to actually use the into_iter trait method to turn the argument into an Iterator.

This is how almost all conversion trait bounds are used (AsRef<T>, Into<T>, etc). You use the trait bound to accept a range of types and use the trait method inside the function to get parameter converted into the type you want. I hope this helps :)

[anna-liao]

@ludwigpacifici @budziq Ah thanks for the explanation! I have tested the code as suggested, and updated the PR

[budziq]

Nice! One final nit and we are ready to merge!

[budziq]

Nice! One final nit and we are ready to merge!

[budziq]

this import is unused leading to warnings.

[budziq]

I'm not sure if we should not replace this multiline Some(Version{...}) with just Some(Version::parse("1.0.0")?) for brevity in both cases (also reducing number of required imports). But it will not be a disaster if we leave it as it is. What are your thoughts on it @anna-liao , @ludwigpacifici ?

[anna-liao]

@budziq I think that's a good idea, as it is more concise and readable. I have updated the PR with the revision.

[ludwigpacifici]

I have one question below, otherwise looks good to me!

[ludwigpacifici]

The semantic of the return type Result<Option<T>> looks off to me. However, I have no better suggestion. Does it mean something specific?

[budziq]

Does it mean something specific?

Yep. Typically Result is used to handle possibility of error while Option expresses possibility of nonexistence. So as long as nonexistence of searched value is not considered a hard error it would be best expressed as an Option while having to deal with parsing errors leaves us with Result so Result<Option<T>> gives the reader of our API a clear indication that the operation can both fail as well as yield no usable return value even if there is no error.

[ludwigpacifici]

Thanks @budziq for your explanation. I appreciate it!

[budziq]

Nicely done @anna-liao !
Thanks for the help with the review @ludwigpacifici !

[ludwigpacifici]

Well done @anna-liao !
You are welcome @budziq !

[anna-liao]

Thanks @budziq and @ludwigpacifici :)