Skip to content

Doc and cosmetics #1395

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
Merged
merged 2 commits into from
Oct 9, 2018
Merged

Doc and cosmetics #1395

merged 2 commits into from
Oct 9, 2018

Conversation

Zen-CODE
Copy link
Member

@Zen-CODE Zen-CODE commented Oct 7, 2018 •
edited
Loading

Although there are many changes here, they are all cosmetic and should not affect any behavior. I know it's not painful to make large and sweeping changes unnecessarily, but I think these are important for readability + pep8 compliance.

The types of changes and rationale are outlines below:

  • Replace '''doc''' with """doc""" -> Pep8 specifies double quotes for doc strings
  • Enforced 80 col line with -> Pep 8
  • Removed unused import (imp) -> Redundant
  • Removed assignment to unused variables -> Redundant
  • Made argument passing indentation consistent
  • Removed brackets around simple strings -> Redundant
  • Made _read_configuration a static method as it does not use self -> good pratice? Or not?
  • Replace using "args" parameter with "_args" -> Convention to indicate non-usage

AndreMiras
AndreMiras reviewed Oct 7, 2018
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.

Nice PR, thanks for it. I'm just not sure why was f017eb9 sneaked in?
Also can we update the tox.ini to reflect the styles you fixed?

pythonforandroid/bootstraps/sdl2/build/build.py Outdated
@@ -410,7 +410,7 @@ def make_package(args):

def parse_args(args=None):
global BLACKLIST_PATTERNS, WHITELIST_PATTERNS, PYTHON
default_android_api = 12
default_android_api = 19
Copy link
Member

Choose a reason for hiding this comment

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

Why is this change in this PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because I branched off the set_app_platform_19 branch. Will remove before I ask for a review.

pythonforandroid/bootstraps/sdl2/build/jni/Application.mk Outdated
@@ -5,3 +5,4 @@

# APP_ABI := armeabi armeabi-v7a x86
APP_ABI := $(ARCH)
APP_PLATFORM := android-19
Copy link
Member

Choose a reason for hiding this comment

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

Same here, I don't think that change should be part of a "cosmetic changes" PR. What do you guys think?

Copy link
Member

Choose a reason for hiding this comment

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

I agree, this should not be added at all without an actual framework around what its value should be and how the user should control it. Setting it to a fixed value will work in some circumstances, but break others.

Copy link
Member Author

Choose a reason for hiding this comment

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

This pr was labeled a WIP because it's a work in progress. It's this not a known protocol? Not ready to be merged because i am aware of these issues. When it's ready i will remove that WIP prefix :-)

Copy link
Member

Choose a reason for hiding this comment

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

Yep I missed that one, cheers 👍

@Zen-CODE Zen-CODE changed the title [WIP] Doc and cosmetics Doc and cosmetics Oct 9, 2018
@Zen-CODE
Copy link
Member Author

Zen-CODE commented Oct 9, 2018

@inclement, @AndreMiras
Okay, the PR is ready now. Re: the suggestion to update the tox.ini. Surely changing that would affect all python files. I have not combed through other files here, just changed them where I came across them in efforts to understand how to handle the 'project.properties' file when building different dists. Surely the tox.ini should only be updated when ensuring checks can be applied project wide?

@AndreMiras
Copy link
Member

Thanks @Zen-CODE yes I was asking for tox, but I wasn't sure if you fixed per error cross project or just per file. So indeed he would complain and we can leave it as it is then.
I would be happy to approve, but it seems that we still have conflict changes on pythonforandroid/toolchain.py. At least that's what GitHub says.
Please ping me when resolved

AndreMiras
AndreMiras approved these changes Oct 9, 2018
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.

Looking good, thanks!
Could you please wait for green build before merging? To be sure to be sure

@Zen-CODE Zen-CODE merged commit ce56784 into master Oct 9, 2018
@Zen-CODE Zen-CODE deleted the doc_and_cosmetics branch October 9, 2018 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@inclement inclement inclement left review comments

@AndreMiras AndreMiras AndreMiras approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Zen-CODE @AndreMiras @inclement