Skip to content
This repository has been archived by the owner on Apr 17, 2023. It is now read-only.

PR with some minor edits #2

Merged
merged 5 commits into from
Sep 1, 2016
Merged

PR with some minor edits #2

merged 5 commits into from
Sep 1, 2016

Conversation

rogerhoward
Copy link
Contributor

Great job in general - nicely commented, your style looks good. Without unnecessary complicating the code (eg., modularizing it, which at this stage wouldn't improve anything), I had only a few suggestions:

  1. Added a shebang to the top of the file, and chmod +x the file, to make it executable without explicitly calling the python interpreter
  2. Changed your string formatting style to the modern style for better forwards compatibility
  3. Added a requirements.txt file, which is the convention for declaring (and installing) dependencies such as NLTK

A couple other notes:

  1. I would never recommend using pip install without using a Python virtualenv, but I didn't want to bikeshed too much
  2. You could handle the setup steps - particularly the nltk.download - automatically, otherwise (I know from experience) this will be skipped and confusing may occur.

Hope this helps!

@tistre tistre merged commit 2dd04c8 into tistre:master Sep 1, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants