Skip to content

Add sieve generator#454

Merged
Insti merged 1 commit intoexercism:masterfrom
pvcarrera:add-sieve-generator
Oct 17, 2016
Merged

Add sieve generator#454
Insti merged 1 commit intoexercism:masterfrom
pvcarrera:add-sieve-generator

Conversation

@pvcarrera
Copy link
Copy Markdown
Contributor

Sieve is the list of exercises that need a generator #396.

When #451 gets merge, I'll rebase this PR and remove the bin/generate-sieve file.

Comment thread exercises/sieve/sieve_test.rb Outdated
953, 967, 971, 977, 983, 991, 997
]
def test_find_primes_up_to_1000
expected = [2, 3, 5, 7, 11, 13, 17, 19, 23, 29, 31, 37, 41, 43, 47, 53, 59, 61, 67, 71, 73, 79, 83, 89, 97, 101, 103, 107, 109, 113, 127, 131, 137, 139, 149, 151, 157, 163, 167, 173, 179, 181, 191, 193, 197, 199, 211, 223, 227, 229, 233, 239, 241, 251, 257, 263, 269, 271, 277, 281, 283, 293, 307, 311, 313, 317, 331, 337, 347, 349, 353, 359, 367, 373, 379, 383, 389, 397, 401, 409, 419, 421, 431, 433, 439, 443, 449, 457, 461, 463, 467, 479, 487, 491, 499, 503, 509, 521, 523, 541, 547, 557, 563, 569, 571, 577, 587, 593, 599, 601, 607, 613, 617, 619, 631, 641, 643, 647, 653, 659, 661, 673, 677, 683, 691, 701, 709, 719, 727, 733, 739, 743, 751, 757, 761, 769, 773, 787, 797, 809, 811, 821, 823, 827, 829, 839, 853, 857, 859, 863, 877, 881, 883, 887, 907, 911, 919, 929, 937, 941, 947, 953, 967, 971, 977, 983, 991, 997]
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.

This generated array is displayed in one line. Should we generate a formatted array with line breaks?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

That would be nice.

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.

Yes, please.

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.

👍 I'll take a look at it tomorrow.

@pvcarrera pvcarrera force-pushed the add-sieve-generator branch from 6159a5e to 6f07656 Compare October 14, 2016 13:32
@kotp kotp mentioned this pull request Oct 14, 2016
@kotp
Copy link
Copy Markdown
Member

kotp commented Oct 14, 2016

Hopefully can squash these commits to one commit, but what ever is logical.

@pvcarrera pvcarrera force-pushed the add-sieve-generator branch from 6f07656 to 17bd48c Compare October 15, 2016 15:03
@pvcarrera
Copy link
Copy Markdown
Contributor Author

@kotp sure, I've merged master, removed the old generate-sieve and squashed the previous commits into one.

Comment thread lib/sieve_cases.rb
OPEN_ARRAY = "[\n\s\s\s\s\s\s".freeze
CLOSE_ARRAY = "\n\s\s\s\s]".freeze
NEW_ARRAY_ROW = ",\n\s\s\s\s\s\s".freeze
ARRAY_ELEMENTS_PER_ROW = 17.freeze
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is 17 the right number?

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.

In the previous sieve_test.rb, the test_primes_up_to_1000 expected array had 17 elements in its first row, so I took this as the base number. The ideal solution should probably count characters and match with Rubocop rules but I wasn't sure how to do it, so I came up with this solution.

@Insti Insti merged commit e2dcf2f into exercism:master Oct 17, 2016
@Insti
Copy link
Copy Markdown
Contributor

Insti commented Oct 17, 2016

Thanks for your work on this @pvcarrera ❤️

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.

3 participants