nth-prime: Provide a function template

[cbzehner]

I found it a bit confusing when I pulled down this exercise and didn't have a template for the function. Ended up pulling the function name and type out of the tests/nth-prime.rs file but I think having this in place would help myself and others.

One alternate approach would be to use a return type of Result<usize, &'a str> to allow Err("Some message") values.

The full function definition would then be:

pub fn nth<'a>(n: usize) -> Result<usize, &'a str> {
    unimplemented!()
}

I wasn't sure if this was better, worse or of no consequence since it introduces the 'a lifetime annotations as well.

I tried to make the minimum number of changes here and ensured that _test/check-exercises.sh was able to validate the nth-prime exercise before submitting.

[coriolinus]

@cbzehner Thank you for this PR! Again, good idea, with a few points which I'd like to see changed before merging, listed below.

[coriolinus]

rustc should complain when asked to compile this stub, because n isn't actually used in the function. (rustc doesn't know that this is a stub function designed to be replaced by students!)

While we don't have tooling in place which requires that the stubs and examples compile without warnings, the goal is to get there eventually. As such, please change n to either _ or _n.

[coriolinus]

WRT the error type of the result, I'm of two minds.

On the one hand, Result<T, ()> is bad style: if you can get away with an empty tuple as your error type, you should be using Option<T> instead.

On the other hand, it's legitimate to want to familiarize students with Result, even if we actually don't care what error type they use.

On the gripping hand, this exercise is number 6 of currently 80 exercises in Rust, per

$ jq '{slug: .exercises[].slug}' config.json | grep slug | nl | grep nth-prime
     6    "slug": "nth-prime"

Students attempting this exercise are still expected to be in the early stages of Rust learning. This implies that we do want to provide a stub for them, and do not want them to need to deal with traits, generics, or other advanced features.

I think the signature I'd like to see looks like this:

pub fn nth(_: usize) -> Result<usize, &'static str> {
    unimplemented!()
}

('static is a special lifetime for things which are known to last the entire lifetime of the program, such as string literals.) This still allows the Err("Some message") usage without requiring people to write a generic lifetime just yet.

However, I'm not 100% convinced of my own correctness yet in this case, and would like to invite comments from other maintainers about this proposal.

[petertseng]

I agree with not showing Result<T, ()> early.

Now, it does lead me to suggest a possible option: Let's just change this execise to use Option instead of Result. Don't need Result yet.

Result<usize, &'static str> is OK to start. I'll attempt to predict some possible objections to them and respond:

• students may not know what &'static is.
  • Okay, then maybe we can explain it like we did in https://github.com/exercism/rust/blob/master/exercises/hello-world/src/lib.rs#L1
• it's bad style to use a string for an error; instead it should be a type that the caller can inspect to determine the cause of the error
  • Okay, maybe we can set the example to use a NoSuchPrime type, that's fine. Should the test suite explicitly check for that type, OR should it just look for is_err?

[coriolinus]

I'd be fine with just using Option<T>. That broadens the scope of this PR, but in a way that I'd be willing to accept.

@cbzehner would you be willing to rework the exercise such that tests expect an Option?

[cbzehner]

Yes, I'd be happy to.

[cbzehner]

Updated this exercise to use Option instead of Result and added a mock implementation where unimplemented uses the passed in variable, similar to what we discussed in #438

[petertseng]

If leaving this as one commit, it will be necessary that the message describes what the commit does, so it should mention that we change from Result to Option.

I would most prefer it if one commit changes from Result to Option and a second commit adds the function definition, so that the former may be reverted (if we choose to do so in the future) without being forced to revert the second.

[petertseng]

For the purpose of providing a more useful test failure to an implementation that returns Some here, I think assert_eq!(..., None) would be better here. If you could examine the resulting error messages to confirm, I will be grateful.

[coriolinus]

I'm now happy with the code as written. Thanks again @cbzehner!

Next steps: Either the commit should be split into two, one which adds the template and another which adjusts the return type from Result to Option, or the commit message should be adjusted to indicate the two changes which were made in the single commit.

I don't have a strong opinion on whether assert!() or assert_eq!() produces a more useful error message; I'm willing to let you choose whether or not to make that adjustment.

I'm now going to leave this for a few days, to ensure that all maintainers have had the chance to look at this and make comments as desired. Unless there are blocking issues discovered, I intend to merge this on 2 March. If after that date I still haven't, please feel free to leave a comment to ping me and I'll take care of it.

[cbzehner]

Thanks, I'll try and split the code into two commits today or tomorrow!

[cbzehner]

Changing this from assert! to assert_eq! was a good call, the error message goes from:

---- test_zeroth_prime stdout ----
	thread 'test_zeroth_prime' panicked at 'assertion failed: np::nth(0).is_none()', tests/nth-prime.rs:28:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

to the much clearer

---- test_zeroth_prime stdout ----
	thread 'test_zeroth_prime' panicked at 'assertion failed: `(left == right)`
  left: `Some(1)`,
 right: `None`', tests/nth-prime.rs:28:5
note: Run with `RUST_BACKTRACE=1` for a backtrace.

You can see the full output of both options here.

[petertseng]

yup, seems good. Thanks for looking into the assert_eq and for splitting into two commits!

[cbzehner]

Thanks for the help and suggestions y'all! I really appreciate all the work you put in getting this to a merge-ready state!

[petertseng]

And thank you as well!