Skip to content

prime-factors: Add solution template #440

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
Mar 7, 2018

Conversation

nathanielknight
Copy link
Contributor

No description provided.

Copy link
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.

@@ -0,0 +1,3 @@
pub fn factors(n: u32) -> Vec<u32> {
unimplemented!()
Copy link
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
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
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
Contributor Author

Cool, thanks! 🙂

Copy link
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

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
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
Contributor Author

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

@coriolinus
Copy link
Member

coriolinus commented Mar 14, 2018 via email

@nathanielknight
Copy link
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
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