srlearn / rnlp

Relational NLP: Convert text into relational facts.
https://rnlp.readthedocs.io/en/latest/
GNU General Public License v3.0
9 stars 5 forks source link

Setup File Corrections #16

Closed hayesall closed 6 years ago

hayesall commented 6 years ago

This PR:

codecov-io commented 6 years ago

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%> (ø) :arrow_up:

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.

skinn009 commented 6 years ago

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

skinn009 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

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 commented 6 years ago

Okay. Thanks for schooling me!

Sent from my iPad

On Aug 3, 2018, at 5:01 PM, Alexander L. Hayes notifications@github.com wrote:

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.

— You are receiving this because your review was requested. Reply to this email directly, view it on GitHub, or mute the thread.