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

Support more kinds of mypy messages #343

Merged
merged 1 commit into from
Jul 24, 2019

Conversation

rik
Copy link
Contributor

@rik rik commented Jul 21, 2019

Extracts a format_message to help unit testing

fix #339

@rik
Copy link
Contributor Author

rik commented Jul 21, 2019

The mypy dependency is the reason the tests are failing. The tests don't use mypy at all but being in the same file, the import fails.

Some options:

  • Move import mypy into MypyTool.__init__
  • Move format_message into another file.
  • Install mypy in test environments (and figure how to do it)

@chocoelho Do you have any preference on how to move forward here?

@chocoelho
Copy link
Contributor

Can you point the PR to develop? We might still have chance to ship it in 1.1.7 and before that have a 1.1.7.dev1.

@rik rik changed the base branch from master to develop July 21, 2019 14:57
@rik
Copy link
Contributor Author

rik commented Jul 21, 2019

That's done. Any opinion on how to pass tests?

@chocoelho
Copy link
Contributor

@rik can you try updating .travis.yml install option to add mypy? Prob worth moving line 17 into 13 in this file and install only once.

@rik rik force-pushed the 339-mypy-messages branch 3 times, most recently from 8ca9e4c to 387b025 Compare July 21, 2019 16:22
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 77.266% when pulling 387b025 on rik:339-mypy-messages into 6d97770 on PyCQA:develop.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 77.266% when pulling 387b025 on rik:339-mypy-messages into 6d97770 on PyCQA:develop.

@coveralls
Copy link

coveralls commented Jul 21, 2019

Coverage Status

Coverage increased (+0.3%) to 78.267% when pulling f1eeb92 on rik:339-mypy-messages into 6d97770 on PyCQA:develop.

@rik
Copy link
Contributor Author

rik commented Jul 21, 2019

The 2.7 failure seems to be https://sourceforge.net/p/docutils/bugs/365/.

@chocoelho
Copy link
Contributor

@carlio as Python 2 support is being handled in https://github.com/landscapeio/prospector2 and discussed in here #265, do you think it makes sense dropping it from TravisCI?

@rik
Copy link
Contributor Author

rik commented Jul 22, 2019

So docutils has been fixed but the 2.7 build is still failing because mypy is not installed on 2.7 builds.

@carlio
Copy link
Member

carlio commented Jul 22, 2019

@chocoelho ahem "handled" I had largely ignored it :-) but yes the idea was that prospector is py3 only as of that point and prospector2 will deal with anyone stuck using 2.

@chocoelho
Copy link
Contributor

@rik there are two approaches we can take here:

  1. check for the python version before the tests which could lead to a bunch of repeated code just to check this
  2. implement a decorator that would behave as a skipIf, just like pytest does (IIRC, unittest does have a skip decorator of some sort but I'm not sure whether it would work for test classes or if nosetest would respect it)

Extracts a `format_message` to help unit testing

fix landscapeio#339
@rik rik marked this pull request as ready for review July 22, 2019 16:44
@chocoelho chocoelho self-requested a review July 22, 2019 20:19
Copy link
Contributor

@chocoelho chocoelho left a comment

Choose a reason for hiding this comment

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

LGTM 👍
Thanks for the contribution, will add you to the contributors list in develop branch.

@chocoelho chocoelho merged commit 2734c98 into landscapeio:develop Jul 24, 2019
@rik
Copy link
Contributor Author

rik commented Jul 24, 2019

Thanks for maintaining this very useful tool!

@rik rik deleted the 339-mypy-messages branch July 24, 2019 11:47
@chocoelho
Copy link
Contributor

You're welcome! We just published 1.1.7.dev1 with this fix, hope we release 1.1.7 soon.

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