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

General maintenance #10

Merged
merged 7 commits into from Dec 11, 2018
Merged

General maintenance #10

merged 7 commits into from Dec 11, 2018

Conversation

quantophred
Copy link
Contributor

This PR:

  • Fixes the tests to work with Click==7.0;
  • Resolves several DeprecationWarnings;
  • Ignores a third-party warning;
  • Fixes a few flake8 errors;
  • Adds Python 3.6 and 3.7 to the build.

The automatic `Try "<command> --help" for help.` output text was added
in Click 7.0.

This commit changes the tests to only check the portion of the output
under our control.
Apparently Travis still only offers 3.7-dev. We should fix this once a
stable 3.7 is available.
Resolves the following two DeprecationWarnings from traitlets 4.1:

    DeprecationWarning: Traits should be given as instances, not types
    (for example, `Int()`, not `Int`). Passing types is deprecated in
    traitlets 4.1.

    DeprecationWarning: metadata {'example': 97} was set from the
    constructor. With traitlets 4.1, metadata should be set using the
    .tag() method, e.g., Int().tag(key1='value1', key2='value2')
@quantophred quantophred changed the title General maintenence General maintenance Dec 6, 2018
Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

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

Took a quick pass at this. Just had a few questions.

.travis.yml Outdated Show resolved Hide resolved
@@ -10,3 +10,6 @@ commands=
[pytest]
addopts = --pep8 --cov straitlets --cov-report term-missing --cov-report html
testpaths = straitlets
filterwarnings =
# PyYAML==3.13
ignore:Using or importing the ABCs:DeprecationWarning:yaml.constructor:126
Copy link
Contributor

Choose a reason for hiding this comment

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

What are these two warnings about?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only one warning, from PyYAML. Full output:

straitlets/ext/tests/test_click.py::test_yaml_file
  site-packages/yaml/constructor.py:126: DeprecationWarning: Using or importing the ABCs from 'collections' instead of from 'collections.abc' is deprecated, and in 3.8 it will stop working
    if not isinstance(key, collections.Hashable):

We require pyyaml>=3.11, so it should go away once it's fixed upstream. There's a couple open PRs with a fix already.

Apparently there is a Situation with PyYAML releases that we might want to keep an eye on, though: see yaml/pyyaml#193, yaml/pyyaml#194.

Copy link

@ernestoeperez88 ernestoeperez88 left a comment

Choose a reason for hiding this comment

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

lgtm

.travis.yml Outdated
@@ -12,3 +14,5 @@ script:
- if [[ $TRAVIS_PYTHON_VERSION = '2.7' ]]; then tox -e py27; fi
- if [[ $TRAVIS_PYTHON_VERSION = '3.4' ]]; then tox -e py34; fi
- if [[ $TRAVIS_PYTHON_VERSION = '3.5' ]]; then tox -e py35; fi
- if [[ $TRAVIS_PYTHON_VERSION = '3.6' ]]; then tox -e py36; fi
- if [[ $TRAVIS_PYTHON_VERSION = '3.7-dev' ]]; then tox -e py37; fi
Copy link
Member

Choose a reason for hiding this comment

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

We could also look at using https://github.com/tox-dev/tox-travis.

Copy link
Contributor

@ssanderson ssanderson left a comment

Choose a reason for hiding this comment

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

LGTM

@quantophred quantophred merged commit e8cadbf into master Dec 11, 2018
@quantophred quantophred deleted the fw-remove-warnings branch December 11, 2018 21:41
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

4 participants