Uh oh!
There was an error while loading. Please reload this page.
parallel-letter-frequency: Implement exercise to resolve #744 - #891
Conversation
ilya-khadykin
left a comment
There was a problem hiding this comment.
Looks solid, good job!
I left some minor comments, please take a look
| import unittest | ||
| from parallel_letter_frequency import calculate | ||
There was a problem hiding this comment.
Could you please also leave a comment stating what version of canonical-data.json the tests were adopted as discussed in #784 if aplicable?
The format is:
# Tests adapted from `problem-specifications//canonical-data.json` @ v1.0.0
There was a problem hiding this comment.
I was unable to find a canonical-data.json in https://github.com/exercism/problem-specifications/tree/master/exercises/parallel-letter-frequency. I noticed that there is an open task for creating one (exercism/problem-specifications#574). Should I still add the comment?
There was a problem hiding this comment.
@forgeRW probably. You can point out that there was no canonical-data.json at the time
| def count_letters(queue_of_texts, letter_to_frequency, worker_id): | ||
| while True: |
There was a problem hiding this comment.
Generally it's a bad practice to you use infinite loops, but I think you know it
Maybe something like this:
whilenotqueue_of_texts.task_done():
# do stuffThere was a problem hiding this comment.
Thank you for the feedback. I removed this in my latest commit.
| worker.start() | ||
| threads.append(worker) | ||
| queue_of_texts.join() | ||
| for i in range(total_workers): |
There was a problem hiding this comment.
Just an alternative:
[queue_of_texts.put(None) for_inrange(total_workers)]There is a convention to use _ if the variable isn't really required
For threads
[t.join() fortinthreads]
cmccandless
left a comment
There was a problem hiding this comment.
@forgeRW Just one minor formatting change, then you should be good!
| The letters used consists of ASCII letters `a` to `z`, inclusive, and is case | ||
| insensitive. | ||
| ### Submitting Exercises |
ilya-khadykin
commented
Nov 7, 2017
@cmccandless let's merge it as is and make minor corrections you mentioned ourselves? How does it sound? |
cmccandless
commented
Nov 7, 2017
AtelesPaniscus
commented
Nov 9, 2017
Hello, This appears to be a level 5 exercise inserted between two level 1 exercises in config.json. Probably something to do with alphabetical ordering. Is this what is wanted ? I'm still working my way through level 1 exercises. Can I skip this beast and come back to it in 10 years time or should I just abandon this track ? |
ilya-khadykin
commented
Nov 10, 2017
@AtelesPaniscus thanks a lot for your input! Of course you can do that. |
…exercism#891) * parallel-letter-frequency: Implement exercise to resolveexercism#744 * parallel-letter-frequency: Removed infinite loop * parallel-letter-frequency: README format fixes
…exercism#891) * parallel-letter-frequency: Implement exercise to resolveexercism#744 * parallel-letter-frequency: Removed infinite loop * parallel-letter-frequency: README format fixes
Resolves#744. Ready for another review. Thanks.