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.

Sign up for GitHub

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

Jump to bottom

chore : Fixed code quality issues #1300

Merged
merged 1 commit into from
Mar 4, 2021
Merged

chore : Fixed code quality issues #1300

merged 1 commit into from
Mar 4, 2021

Conversation

powerexploit
Copy link
Contributor

@powerexploit powerexploit commented Feb 27, 2021 •
edited
Loading

Description

Hi 👋 I ran the DeepSource static analyzer on the forked copy of this repo and found some interesting code quality issues. This PR fixes a few of them.

Summary of Fixes

  • Added .deepsource.toml to fix bug risks
  • Remove unnecessary use of comprehension
  • Use os.environ.copy() to copy the environment variables
  • Remove unnecessary return statement
  • Use sys.exit() calls

Type of change

  • Antipattern
  • New feature (non-breaking change which adds functionality)

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation

@powerexploit
Fixed code quality issues using deepsource
bb03805 
- Added .deepsource.toml to fix bug risks

- Remove unnecessary use of comprehension

- Use `os.environ.copy()` to copy the environment variables

- Remove unnecessary return statement

- Use `sys.exit()` calls
@powerexploit powerexploit marked this pull request as ready for review February 27, 2021 06:53
@powerexploit
Copy link
Contributor Author

@AndreMiras kindly review .

AndreMiras
AndreMiras approved these changes Mar 2, 2021
View reviewed changes
Copy link
Member

@AndreMiras AndreMiras left a comment

Choose a reason for hiding this comment

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

Thanks for the PR 🙏
I'm just unsure about the bundling the .deepsource.toml file here since we don't use it with our CI and already have flake8.

buildozer/targets/osx.py
@@ -55,7 +55,7 @@ def download_kivy(self, cwd, py_branch=2):
"Unable to download the Kivy App. Check osx.kivy_version in your buildozer.spec, and verify "
"Kivy servers are accessible. http://kivy.org/downloads/")
check_call(("rm", "Kivy{}.dmg".format(py_branch)), cwd=cwd)
exit(1)
sys.exit(1)
Copy link
Member

Choose a reason for hiding this comment

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

It wasn't clear to me why this wasn't caught earlier at least by flake8, but actually site.exit seems to be imported by default. Using the correct sys.exit is not a bad idea still, so thanks

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @AndreMiras .

.deepsource.toml
@@ -0,0 +1,25 @@
version = 1
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what to think about having that linter config there, but not using it anywhere except in your local setup.
Also I don't know about adding another linter when we already have flake8.
Not completely against, simply thinking out loud

buildozer/targets/osx.py
@@ -76,8 +76,6 @@ def ensure_kivyapp(self):
else:
self.download_kivy(kivy_app_dir, py_branch)

return
Copy link
Member

Choose a reason for hiding this comment

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

I've always been bugged by that, but I think we have more in the code base here or in p4a so I didn't dare to touch it 😅
I'm happy to remove it still if it comes from someone else 😛

buildozer/__init__.py Show resolved Hide resolved
@powerexploit
Copy link
Contributor Author

Thanks for the PR
I'm just unsure about the bundling the .deepsource.toml file here since we don't use it with our CI and already have flake8.

Thanks @AndreMiras !
If you don't want to include .deepsource.toml file so I will remove it apart from approved fixes.

@AndreMiras
Copy link
Member

I'm curious to know what @tshirtman thinks about it regarding the .deepsource.toml.
If he thinks it's no big deal then he can directly merge the PR.
My feeling is in 1 year from now we could see this file and think it's a left over from a tool we no longer use since it doesn't appear anywhere

@tshirtman
Copy link
Member

I don't really have an opinion on that, if we it helps keeping track of future similar issues through the service, and we plan to maybe integrate that in the CI, then why not? :)

@tshirtman tshirtman merged commit d2944ad into kivy:master Mar 4, 2021
@powerexploit
Copy link
Contributor Author

Thanks @AndreMiras @tshirtman .

Activating analysis will help you to continuously analyze repo for code quality issues and fix commonly occurring issues using the auto-fix feature.

I would like to share some steps to activate DeepSource :

  • Install DeepSource on your repository here.
  • create .deepsource.toml configuration which you can use to configure your analysis settings.

Note: This PR already have .deepsource.toml file so after merging you don't need to create this one.

  • Activate analysis here.

@powerexploit
Copy link
Contributor Author

Thanks @AndreMiras @tshirtman .

Activating analysis will help you to continuously analyze repo for code quality issues and fix commonly occurring issues using the auto-fix feature.

I would like to share some steps to activate DeepSource :

  • Install DeepSource on your repository here.
  • create .deepsource.toml configuration which you can use to configure your analysis settings.

Note: This PR already have .deepsource.toml file so after merging you don't need to create this one.

  • Activate analysis here.

Hey , @AndreMiras , @tshirtman Please have a look.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tshirtman tshirtman tshirtman left review comments

@AndreMiras AndreMiras AndreMiras approved these changes

@AndiEcker AndiEcker AndiEcker left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@powerexploit @AndreMiras @tshirtman @AndiEcker