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.py cleanup and mypy #48

Merged
merged 6 commits into from
Jul 28, 2020
Merged

setup.py cleanup and mypy #48

merged 6 commits into from
Jul 28, 2020

Conversation

jayqi
Copy link
Member

@jayqi jayqi commented Jul 26, 2020

  • Add MIT license
  • Clean up and polish for setup.py, removing some deprecated options
  • Explicitly mention the code review use case in README and help description text.
  • Add mypy type checking, address some of the errors
  • Remove .travis.yml

@jayqi jayqi requested review from r-b-g-b, pjbull and ejm714 July 26, 2020 01:38
@github-actions
Copy link
Contributor

github-actions bot commented Jul 26, 2020

@codecov
Copy link

codecov bot commented Jul 26, 2020

Codecov Report

Merging #48 into master will not change coverage.
The diff coverage is 100.0%.

@@          Coverage Diff           @@
##           master     #48   +/-   ##
======================================
  Coverage    98.1%   98.1%           
======================================
  Files           8       8           
  Lines         371     371           
======================================
  Hits          364     364           
  Misses          7       7           
Impacted Files Coverage Δ
nbautoexport/nbautoexport.py 98.2% <ø> (ø)
nbautoexport/clean.py 100.0% <100.0%> (ø)
nbautoexport/export.py 94.9% <100.0%> (ø)
nbautoexport/sentinel.py 100.0% <100.0%> (ø)

name="nbautoexport",
packages=find_packages(include=["nbautoexport", "nbautoexport.*"]),
setup_requires=setup_requirements,
test_suite="tests",
tests_require=test_requirements,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just for my own learning, why don't we need lines 60-62?

Copy link
Member Author

@jayqi jayqi Jul 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://setuptools.readthedocs.io/en/latest/setuptools.html This page from the setuptools docs is where this stuff is described.

setup_requires has a red warning box that says it's discouraged in favor of the PEP-517/PEP-518 new system using pyproject.toml that we ran into with sample-weights. We also don't need it for this package, because we don't have any special build-time dependencies.

test_suite and tests_require are both for if we run our tests with python setup.py tests. We don't run our tests that way anyways, and "New in 41.5.0: Deprecated the test command" is everywhere in the setuptools docs that has anything to do with running tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TIL 🙇

Copy link
Contributor

@ejm714 ejm714 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! 🙇

@pjbull pjbull merged commit 58ab28d into master Jul 28, 2020
@pjbull pjbull deleted the misc branch July 28, 2020 15:33
@jayqi jayqi mentioned this pull request Jul 30, 2020
3 tasks
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.

None yet

3 participants