Skip to content
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

Fix CONTRIBUTING.md #2198

Merged
merged 2 commits into from Dec 17, 2018

Conversation

Projects
None yet
3 participants
@NQNStudios
Copy link
Contributor

commented Nov 30, 2018

This simply changes the documentation so other contributors won't run into my problem with #2195.

@purificant

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

Should the docs specify all for data downloads, or should we amend tests to actually include all the necessary data for tests?

Any thoughts @alvations?

@NQNStudios

This comment has been minimized.

Copy link
Contributor Author

commented Dec 3, 2018

@purificant

This comment has been minimized.

Copy link
Contributor

commented Dec 3, 2018

This is a valid issue, as any new contributor is likely to run into it by following the docs. Hat off to @NQNStudios for identifying the issue and proposing a fix.

If python -m nltk.downloader tests is replaced with python -m nltk.downloader all in the docs, will that essentially make tests obsolete? Why was only a subset of data installed for tests previously and not all of the available data?

@NQNStudios

This comment has been minimized.

Copy link
Contributor Author

commented Dec 11, 2018

Would you like me to measure how much file size is added by using all instead of tests? In any case, I'm also happy to update the tests resource list to add what's needed, or remove it if all is the way to go.

@alvations

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

@purificant @NQNStudios @Copper-Head yes, since the CI is already using nltk.download('all') https://github.com/nltk/nltk/blob/develop/tools/travis/install.sh#L10 , it makes sense that contributors should start testing with a similar environment as the CI.

Until we find a better way to handle and separate test-needed data from all, downloading all nltk_data seems to be necessary for contributors to get through the testing with ease.

@alvations alvations self-assigned this Dec 17, 2018

@alvations alvations merged commit 985b1e2 into nltk:develop Dec 17, 2018

1 of 2 checks passed

NLTK tests Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alvations

This comment has been minimized.

Copy link
Contributor

commented Dec 17, 2018

Thanks @NQNStudios for raising this! Thanks @purificant for following up!

@NQNStudios NQNStudios deleted the NQNStudios:contrib-doc-fix branch Dec 17, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.