koodaamo / tnefparse

a TNEF decoding library written in python, without external dependencies
GNU Lesser General Public License v3.0
49 stars 37 forks source link

Remove obsolete `setup_requires` entry #61

Closed jugmac00 closed 4 years ago

jugmac00 commented 4 years ago

... which also broke the Python 2.7 build on GitHub Actions.

This fixes #59 and replaces #60

modified: setup.py

codecov[bot] commented 4 years ago

Codecov Report

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

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #61   +/-   ##
=======================================
  Coverage   92.98%   92.98%           
=======================================
  Files           7        7           
  Lines        1297     1297           
  Branches      121      121           
=======================================
  Hits         1206     1206           
  Misses         69       69           
  Partials       22       22           

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 7aee417...1924bb9. Read the comment docs.

petri commented 4 years ago

Why is the entry obsolete?

jugmac00 commented 4 years ago

Hi @petri sorry for not being more explicit - I thought the information I gave at the issue #59 would have been enough. https://github.com/koodaamo/tnefparse/issues/59#issuecomment-631312999

If I understand the "history" correct, you added setup_requires in order to be able to use pytest-runner which offers a hook so python setup.py test runs pytest instead of unit test.

python setup.py test is deprecated (see https://setuptools.readthedocs.io/en/latest/setuptools.html scroll down to tests_require) or here https://github.com/pypa/setuptools/issues/1684

Also pytest-runner is deprecated - see https://pypi.org/project/pytest-runner/ The author recommends to remove this entry (and even more) and to use a test runner like tox - which this project already uses. (Also, the author notes that using pytest-runner is a security issue - see the first paragraph of his deprecation warning).

The intention of this PR was just to fix the broken Py27 build.

As I wrote in #59 I planned to have several PRs - this one here, and another one getting rid of the other now obsolete (because pytest-runner is deprecated) entries and to get rid of python setup.py test and replace it by pytest only.

The best argument for being obsolete is - as I removed the entry - nothing happened. Also, the coverage percentage is still the same.

If there is a use case I might have overlooked, just close the PR.

petri commented 4 years ago

Oh. Thanks for a very thorough answer! Your analysis of history is accurate. I did not realize the stuff was deprecated. Merging.