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

More edits to index, README #15

Closed skinn009 closed 6 years ago

skinn009 commented 6 years ago

Alerted users that nltk must be installed. My list of needed packages may be incomplete, ambiguous, or redundant.

codecov-io commented 6 years ago

Codecov Report

Merging #15 into master will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #15   +/-   ##
=======================================
  Coverage   98.26%   98.26%           
=======================================
  Files           7        7           
  Lines         231      231           
=======================================
  Hits          227      227           
  Misses          4        4

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 780c270...073c1ec. Read the comment docs.

hayesall commented 6 years ago

@leodd is working on getting an install_requires keywords in the setup file to simplify this a bit further, but having the explicit instructions as part of the documentation would be good to have.

.. code-block:: python

  import nltk
  nltk.download('punkt')
  nltk.download('stopwords')
  nltk.download('averaged_perceptron_tagger')
skinn009 commented 6 years ago

Will do.

On Fri, Aug 3, 2018 at 12:12 PM, Alexander L. Hayes < notifications@github.com> wrote:

@batflyer requested changes on this pull request.

Maybe a code block showing how to install the dependencies?

I pulled these out of the top of the current setup.py, having them in a similar format to this might make it a little more clear.

import nltk nltk.download('punkt') nltk.download('stopwords') nltk.download('averaged_perceptron_tagger')

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/starling-lab/rnlp/pull/15#pullrequestreview-143263359, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQ7T-SOHKwFou98jRrkDobz87NrL1s7ks5uNISAgaJpZM4VuUH_ .

hayesall commented 6 years ago

These look good, I'm going to squash-and-merge since there were a few incremental changes. Thanks Mike!

skinn009 commented 6 years ago

Alex, I made the changes you requested on my remote. Do we just do another pull request, or do we abort the pending pull?

Mike

On Fri, Aug 3, 2018 at 12:12 PM, Alexander L. Hayes < notifications@github.com> wrote:

@batflyer requested changes on this pull request.

Maybe a code block showing how to install the dependencies?

I pulled these out of the top of the current setup.py, having them in a similar format to this might make it a little more clear.

import nltk nltk.download('punkt') nltk.download('stopwords') nltk.download('averaged_perceptron_tagger')

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/starling-lab/rnlp/pull/15#pullrequestreview-143263359, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQ7T-SOHKwFou98jRrkDobz87NrL1s7ks5uNISAgaJpZM4VuUH_ .

skinn009 commented 6 years ago

Oh- our messages passed in the mail.

On Fri, Aug 3, 2018 at 1:01 PM, Alexander L. Hayes <notifications@github.com

wrote:

These look good, I'm going to squash-and-merge since there were a few incremental changes. Thanks Mike!

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/starling-lab/rnlp/pull/15#issuecomment-410331449, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQ7T5WZsnCfJ6qwL3jDPhUJOll0kKkIks5uNJAGgaJpZM4VuUH_ .

hayesall commented 6 years ago

@skinn009 Adding the additional commits on the same branch as the pull request are also added to be part of the pull request, so all of your changes were merged in herer. I did a "squash merge" on this though since there were a few incremental changes to the same lines of the same files.

skinn009 commented 6 years ago

I may not be understanding your changes- 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.

On Fri, Aug 3, 2018 at 1:17 PM, Alexander L. Hayes <notifications@github.com

wrote:

@skinn009 https://github.com/skinn009 Adding the additional commits on the same branch as the pull request are also added to be part of the pull request, so all of your changes were merged in herer. I did a "squash merge" on this though since there were a few incremental changes to the same lines of the same files.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/starling-lab/rnlp/pull/15#issuecomment-410335638, or mute the thread https://github.com/notifications/unsubscribe-auth/AWQ7T0Bxd1e07ZHeH-tOpH_TsBSmPgabks5uNJPMgaJpZM4VuUH_ .