Skip to content

Linting take 1 #19

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

Merged
merged 2 commits into from
Apr 28, 2018
Merged

Linting take 1 #19

merged 2 commits into from
Apr 28, 2018

Conversation

mattsb42-aws
Copy link
Member

@mattsb42-aws mattsb42-aws commented Apr 27, 2018

Code formatting/style/smell fixes identified by flake8/pylint.

BLOCKED BY #16

BLOCKED BY #18

NOTE

To ease parallelization of efforts, this PR is based on the DELETE-m-1 fork. Prior to looking over this PR, #16 and #18 should be merged to master and the base for this PR be changed to master.

@mattsb42-aws mattsb42-aws changed the title Linting 1 Linting take 1 Apr 27, 2018
@praus praus changed the base branch from DELETE-m-1 to master April 27, 2018 23:31
verifier.verify(signature)
except Exception:
message = 'Unable to verify signature'
_LOGGER.exception(message)
Copy link

Choose a reason for hiding this comment

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

Would it make sense to include the message (perhaps trimmed to a certain length) in the log to help with debugging?

Copy link
Member Author

Choose a reason for hiding this comment

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

When the "exception" logging level is used in the scope of an except block, it automatically logs the full stacktrace of the exception that caused that block to be entered.

Copy link

Choose a reason for hiding this comment

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

But that stack trace wouldn't contain the message its signature that we failed to verify, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a very good point. Opened #26 to track this, because I think that would be very useful.

@mattsb42-aws
Copy link
Member Author

Apparently it also needed to be rebased after master was merged...ok then.

@mattsb42-aws mattsb42-aws merged commit bb4b2d1 into aws:master Apr 28, 2018
@mattsb42-aws mattsb42-aws deleted the linting-1 branch April 28, 2018 00:16
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.

2 participants