Skip to content

prime-factors: Add solution template#440

Merged
coriolinus merged 2 commits intoexercism:masterfrom
nathanielknight:master
Mar 7, 2018
Merged

prime-factors: Add solution template#440
coriolinus merged 2 commits intoexercism:masterfrom
nathanielknight:master

Conversation

@nathanielknight
Copy link
Copy Markdown
Contributor

No description provided.

Copy link
Copy Markdown
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Hi @neganp, thanks for this PR! It generally looks good, but I'd like you to make the change shown below so that there are no compile warnings when the student first downloads the exercise.

Comment thread exercises/prime-factors/src/lib.rs Outdated
@@ -0,0 +1,3 @@
pub fn factors(n: u32) -> Vec<u32> {
unimplemented!()
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please update to reference n so that there is no unused variable warning, like this.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh nice! I didn't know you could do that. 👍

This way, the learner doesn't hit `unused variable` warnings when they
first compile the example.
Copy link
Copy Markdown
Member

@coriolinus coriolinus left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

I'm going to let this sit for a few days now so other maintainers have a chance to take a look and make their comments, but unless blocking changes are discovered, I intend to merge this on 5 Mar.

@nathanielknight
Copy link
Copy Markdown
Contributor Author

Cool, thanks! 🙂

Copy link
Copy Markdown
Member

@petertseng petertseng left a comment

Choose a reason for hiding this comment

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

Yes.

I recommend squash when merging.

You have reminded me (or this PR caused me to remind myself, whichever wording you think is better) that in #269 we decided that all exercises should have stubs that allow running the tests, got it.

@coriolinus coriolinus merged commit 80cc2a5 into exercism:master Mar 7, 2018
@pravic
Copy link
Copy Markdown

pravic commented Mar 14, 2018

@neganp Have you reviewed your code? The test uses a big number which does not fit into u32:

fn test_factors_include_large_prime() {
assert_eq!(factors(93819012551), vec![11, 9539, 894119]);
}

@nathanielknight
Copy link
Copy Markdown
Contributor Author

Oh, good catch. Weird that it passed all my tests 😕

It looks like a u64 is big enough on my platform. Would that work?

@nathanielknight
Copy link
Copy Markdown
Contributor Author

Wait, this doesn't even pass my tests; how embarrassing 😧 .

@coriolinus
Copy link
Copy Markdown
Member

coriolinus commented Mar 14, 2018 via email

@nathanielknight
Copy link
Copy Markdown
Contributor Author

I looks like some of the tests don't run on at least some of the builds: https://travis-ci.org/exercism/rust/jobs/347483074#L2213

@coriolinus
Copy link
Copy Markdown
Member

I think that's a bug which we should fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants