Skip to content

Doc and cosmetics#1395

Merged
Zen-CODE merged 2 commits into
masterfrom
doc_and_cosmetics
Oct 9, 2018
Merged

Doc and cosmetics#1395
Zen-CODE merged 2 commits into
masterfrom
doc_and_cosmetics

Conversation

@Zen-CODE

@Zen-CODE Zen-CODE commented Oct 7, 2018

Copy link
Copy Markdown
Member

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 left a comment

Copy link
Copy Markdown
Member

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?

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

Copy link
Copy Markdown
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
Copy Markdown
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.


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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Zen-CODE commented Oct 9, 2018

Copy link
Copy Markdown
Member Author

@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
Copy Markdown
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 left a comment

Copy link
Copy Markdown
Member

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

Comment thread pythonforandroid/toolchain.py
@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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants