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

Fix missing 'UnknownMessage' exception #207

Merged
merged 2 commits into from
May 2, 2017
Merged

Fix missing 'UnknownMessage' exception #207

merged 2 commits into from
May 2, 2017

Conversation

sijis
Copy link
Contributor

@sijis sijis commented Apr 19, 2017

The recent 1.7 version of pylint moved the exceptions to a different
submodule and this ensures everything works as expected.

Since there's been no activity on #205, I took it upon myself to update it as suggested there.
I did end up moving the pylint exception to the prospector.exceptions submodule, as I found it odd to sprinkle a try .. except everywhere.

I did not update the _INSTALL_REQUIRES in setup.py to up the version of pylint to pylint>=1.7 or so. I think it should probably be done too.

This should also resolve #206.

The recent 1.7 version of pylint moved the exceptions to a different
submodule and this ensures everything works as expected.
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 59.259% when pulling 13d7164 on sijis:fix/pylint_exception into c893531 on landscapeio:master.

2 similar comments
@coveralls
Copy link

Coverage Status

Coverage increased (+0.09%) to 59.259% when pulling 13d7164 on sijis:fix/pylint_exception into c893531 on landscapeio:master.

@coveralls
Copy link

coveralls commented Apr 19, 2017

Coverage Status

Coverage increased (+0.09%) to 59.259% when pulling 13d7164 on sijis:fix/pylint_exception into c893531 on landscapeio:master.

Copy link

@berkerpeksag berkerpeksag left a comment

Choose a reason for hiding this comment

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

This looks good to me, thank you! Just left a minor comment in prospector/exceptions.py.

Adding a note to CHANGELOG.md may have increase the chance of this PR accepted faster (less work for maintainers)

I'm not a maintainer, but I think _INSTALL_REQUIRES should be updated in a feature release, not in a bugfix (or hotfix) release.

@@ -1,5 +1,8 @@
# -*- coding: utf-8 -*-

try:

Choose a reason for hiding this comment

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

I'd add a comment explaining why this is here to help future reviewers.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point. I'll put some comments in it.

@coveralls
Copy link

coveralls commented Apr 20, 2017

Coverage Status

Coverage increased (+0.09%) to 59.259% when pulling 977ab4b on sijis:fix/pylint_exception into c893531 on landscapeio:master.

@sijis
Copy link
Contributor Author

sijis commented Apr 21, 2017

@carlio Any feedback? I'd like to get this fixed.

@carlio
Copy link
Member

carlio commented Apr 22, 2017

@sijis Hi apologies I am currently on holiday with minimal internet connection, will try to review and merge and release a new version today or tomorrow (PHT)

@sijis
Copy link
Contributor Author

sijis commented Apr 22, 2017

Ok. Great! If there is anything you need adjusted let me know.

@sijis
Copy link
Contributor Author

sijis commented Apr 29, 2017

Have you had an opportunity to review the PR?

@carlio carlio merged commit 7787795 into landscapeio:master May 2, 2017
@carlio
Copy link
Member

carlio commented May 2, 2017

@sijis yes, finally also can upload a new package. Will do so in a few hours. Sorry it took so long.

@sijis
Copy link
Contributor Author

sijis commented May 2, 2017

Thank you!! Totally fine that it took a while to get this in. Vacation/holiday is important.

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.

pylint 1.7.0 missing UnknownMessage
4 participants