dpalmasan / TRUNAJOD2.0

An easy-to-use library to extract indices from texts.
https://trunajod20.readthedocs.io/en/latest/
MIT License
29 stars 7 forks source link

Added type hints to functions in discourse_markers.py [Issue: #16] #19

Closed sourvad closed 3 years ago

sourvad commented 3 years ago

Added type hints to discourse_markers.py (Issue #16). Also added 2 imports to support these types.

sourvad commented 3 years ago

I'm having some trouble with the build succeeding. Travis CI is unable to import spacy. I saw in "discourse_markers_test.py" that there is a workaround for the Doc dependency. However removing the spacy import then has a NameError for Doc in "discourse_markers.py".

dpalmasan commented 3 years ago

Hello @sourvad thanks for helping in this issue. I think what you'd like to update is the tox.ini file, because tests run in an isolate environment using tox. So, in testenv property, you need to add spacy as dependency, and you should see no issue if you just run tox from root directory:

[tox]
envlist = py36

[pytest]
norecursedirs = docs *.egg-info .git appdir .tox

[testenv]
deps =
    pytest
    coverage
    pytest-cov
    spacy
    mock
commands = pytest

Example:

from spacy.tokens import Doc
from typing import List

# Code

# Example of updated function
def find_matches(text: Doc, list: List[str]):
    """Return matches of words in list in a target text.

    Given a text and a list of possible matches (in this module, discourse
    markers list), returns the number of matches found in text. This ignores
    case.

    .. hint:: For non-Spanish users
       You could use this function with your custom list of discourse markers
       in case you need to compute this metric. In that case, the way to call
       the funcion would be: ``find_matches(YOUR_TEXT, ["dm1", "dm2", etc])``

    :param text: Text to be processed
    :type text: string
    :param list: list of discourse markers
    :type list: Python list of strings
    :return: Number of ocurrences
    :rtype: int
    """
    counter = 0
    for w in list:
        results = re.findall(r"\b%s\b" % w, text, re.IGNORECASE)
        counter += len(results)
    return counter

Then if you run tox from the root dir, it should run successfully without complaining about the spacy dependency. Let me know if this works!

dpalmasan commented 3 years ago

Okay @sourvad I took a look, it turns out that we will have issues when dealing with python 3.6 + tox + using a pyproject.toml + PEP 517. The solution to make the build pass, we will have to remove support from python3.6, but for now, these files should be updated:

pyproject.toml

[tool.black]
line-length = 79
include = '\.pyi?$'
exclude = '''
/(
    \.git
  | \.hg
  | \.mypy_cache
  | \.tox
  | \.venv
  | _build
  | buck-out
  | build
  | dist
)/
'''

[tool.pytest.ini_options]
minversion = "6.0"
addopts = "-ra --cov=. --cov-fail-under=80"
testpaths = [
    "tests"
]

[build-system]
requires = ["setuptools", "wheel"]
build-backend = "setuptools.build_meta"

.travis.yml

language: python
python:
  - 3.7
before_install:
  - python --version
  - pip install -U pip
  - pip install -U pytest
  - pip install codecov
  - pip install -U pytest-cov
install:
  - pip install tox-travis
script: tox

tox.ini

[tox]
envlist = py37

[pytest]
norecursedirs = docs *.egg-info .git appdir .tox

[testenv]
deps =
    pytest
    coverage
    pytest-cov
    mock
    spacy

commands = pytest

After the changes, we should see the build pass. Let me know if you need help.

sourvad commented 3 years ago

@dpalmasan Thank you for your help with the build process. This was my first time contributing to any repo and I was a bit lost on the process.

dpalmasan commented 3 years ago

Sure, @sourvad thanks for contributing. Might I ask just one more thing before merging?

Could you install pre-commit hooks? Steps:

It will do some checks on code (I am not running those on travis). Then after all check pass, you can just do git commit --amend. Thanks in advance!

sourvad commented 3 years ago

Hey @dpalmasan all done!

dpalmasan commented 3 years ago

Looks good! Going to merge it, thanks!