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 the LGTM alerts in setup_win_common.py #1352

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

charlesej
Copy link
Contributor

Overview of changes:

System details:

  • os: windows 10 (64bit)
  • python: 3.7.4 (64bit) and 2.7.10 (64bit)
  • pygame: 2.0.0.dev3 (SDL: 2.0.10) at 1556f34

Resolves the setup_win_common.py alerts from the LGTM link in #1133.

- Fixed the LGTM alerts in setup_win_common.py
  (LGTM info for pygame: https://lgtm.com/projects/g/pygame/pygame)
@lgtm-com
Copy link

lgtm-com bot commented Sep 29, 2019

This pull request fixes 2 alerts when merging 50e54d9 into 1556f34 - view on LGTM.com

fixed alerts:

  • 2 for Should use a 'with' statement

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Seems this takes the pattern matching outside of the try/except as well as using a with context manager for the open. I guess that doesn't matter, as it seems to work.

@illume
Copy link
Member

illume commented Sep 30, 2019

Thanks

@charlesej
Copy link
Contributor Author

Thanks for reviewing the changes.

This was addressing 2 LGTM alerts of type: "Instance of context-manager class file is closed in a finally block. Consider using 'with' statement."

In the get_definitions() function I thought moving that code (it doesn't require the file to be open) outside the with context manager was okay, since the finally block was only being used to close the file.

Probably should have included this information in the initial description of the changes.

@illume
Copy link
Member

illume commented Oct 1, 2019

Thanks for the explanation. Makes sense to me :)

@illume illume merged commit fb728c0 into pygame:master Oct 1, 2019
@charlesej charlesej deleted the fix-setup-win-common-lgtm-alerts branch October 1, 2019 16:08
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

2 participants