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

Setup File Corrections #16

Merged
merged 2 commits into from
Aug 31, 2018
Merged

Setup File Corrections #16

merged 2 commits into from
Aug 31, 2018

Conversation

hayesall
Copy link
Member

@hayesall hayesall commented Aug 3, 2018

This PR:

  • Bumps the version number from 0.3.1 to 0.3.2
  • Overwrites the setup.py install method in order to also download nltk package requirements.

@hayesall hayesall requested a review from leodd August 3, 2018 18:14
@codecov-io
Copy link

codecov-io commented Aug 3, 2018

Codecov Report

Merging #16 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #16   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files           7        7           
  Lines         231      231           
=======================================
  Hits          227      227           
  Misses          4        4
Impacted Files Coverage Δ
rnlp/__init__.py 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4b965e...fe82190. Read the comment docs.

@hayesall hayesall requested a review from skinn009 August 3, 2018 18:43
@skinn009
Copy link
Contributor

skinn009 commented Aug 3, 2018

Is this where I should comment on your code changes?- mas

@skinn009
Copy link
Contributor

skinn009 commented Aug 3, 2018

what if the user decides she doesn't want nltk installed, and would rather not use rnlp if the package is required? I wonder if most users would rather control any package imports.

@hayesall
Copy link
Member Author

hayesall commented Aug 3, 2018

what if the user decides she doesn't want nltk installed, and would rather not use rnlp if the package is required? I wonder if most users would rather control any package imports

I think this is a good point, but we have two somewhat mutually exclusive options:

  1. The package is easy to install (and handles installing all dependencies)
  2. The package is harder to install but checks everything with the user

A good middle ground (in my opinion) would be to list the dependencies (such that a user can inspect them ahead of time), but make the package easy to install.

@skinn009
Copy link
Contributor

skinn009 commented Aug 3, 2018

I wonder if there is another option: the user installs the necessary packages (according to the code instructions we gave in the README). Then it is a simple matter to install rnlp. I guess we need to balance ease of use with user sovereignty, and I of course will defer to your expertise and experience.

@hayesall
Copy link
Member Author

hayesall commented Aug 3, 2018

the user installs the necessary packages (according to the code instructions we gave in the README).

This is basically the same as (2), but manually installing everything gets messy the further down you go. For example, Sphinx requires 12 Python dependencies to be fulfilled (each of which may also have their own dependencies).

setup.py helps manage dependencies and gets someone to the point where they can use the software with (hopefully) the minimal number of packages.

Manually installing specific packages (like sphinx, or unittest) makes sense--since not everyone will be building the documentation from scratch or running unit tests. Other packages (such as nltk) are absolutely required because a person cannot use this software without either rewriting the code or installing nltk.

@skinn009
Copy link
Contributor

skinn009 commented Aug 3, 2018 via email

@hayesall hayesall changed the base branch from master to development August 24, 2018 17:26
@skinn009 skinn009 merged commit b0de66a into srlearn:development Aug 31, 2018
@hayesall hayesall deleted the setup_file branch August 31, 2018 21:16
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