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

Specify Python 2 for LGTM #62

Merged
merged 2 commits into from
Nov 14, 2019
Merged

Conversation

tyll
Copy link
Member

@tyll tyll commented Oct 31, 2019

LGTM defaults to Python 3 for the storage role because it was created
recently. Since it needs to support Python 2 as well, configure it
explicitly.

LGTM is a static code analyzer to identify bugs in code, it found real issues that I fixed in PR #52.


This change is Reviewable

LGTM defaults to Python 3 for the storage role because it was created
recently. Since it needs to support Python 2 as well, configure it
explicitly.
@tyll tyll requested review from dwlehman and pcahyna October 31, 2019 11:18
@tyll tyll mentioned this pull request Oct 31, 2019
@lgtm-com
Copy link
Contributor

lgtm-com bot commented Oct 31, 2019

This pull request introduces 2 alerts when merging fd08008 into 378239c - view on LGTM.com

new alerts:

  • 2 for Property in old-style class

The @Property decorator requires a new-style class in Python 2, see
https://lgtm.com/rules/10030086/
@pcahyna pcahyna requested a review from i386x October 31, 2019 12:51
@pcahyna
Copy link
Member

pcahyna commented Oct 31, 2019

@i386x could you please get it into the CI template (PR linux-system-roles/template#5 )?
@tyll when merging, we will not need the 2nd commit, because it is already included in #51 .

@tyll
Copy link
Member Author

tyll commented Oct 31, 2019

@tyll when merging, we will not need the 2nd commit, because it is already included in #51 .

This seems to be incorporated into #43 now. Since there are no objections afaik, it makes sense to move your commit into its own PR to fix this (#43 is too big IMHO) or just keep the commit in this PR.

@i386x
Copy link
Contributor

i386x commented Nov 4, 2019

@i386x could you please get it into the CI template (PR linux-system-roles/template#5 )?

Done in linux-system-roles/template@427866f.

Copy link
Contributor

@i386x i386x left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Copy link
Collaborator

@dwlehman dwlehman left a comment

Choose a reason for hiding this comment

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

Looks good.

@dwlehman dwlehman merged commit bfa4f29 into linux-system-roles:master Nov 14, 2019
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.

4 participants