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

Add autopep8 script to help rebasing branches after #4440. #4447

Merged
merged 1 commit into from Aug 17, 2017

Conversation

Projects
None yet
4 participants
@jmchilton
Copy link
Member

commented Aug 17, 2017

Fixes a few more PEP8 issues as part of the autopep8'ing as well.

This deliberately targets fewer files than the corresponding linting - I don't want to target extra tools in the tools directory for instance.

Merging this would help me update galaxy-lib and my CWL branch of Galaxy.

@galaxybot galaxybot added the triage label Aug 17, 2017

@galaxybot galaxybot added this to the 17.09 milestone Aug 17, 2017

@jmchilton jmchilton force-pushed the jmchilton:autopep8 branch from 46ec7f8 to 753a256 Aug 17, 2017

raw_tags = trans.app.tag_handler.parse_tags(column_filter.encode("utf-8"))
clause_list = []
for name, value in raw_tags:
if name:
# Filter by all tags.

This comment has been minimized.

Copy link
@dannon

dannon Aug 17, 2017

Member

Should also be dedented? (and below)

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 17, 2017

Author Member

You'd think... let me see if there is a separate rule for comments...

@@ -721,7 +721,7 @@ def __init__(self, params, sanitize=True):
key not in self.NEVER_SANITIZE and
True not in [key.endswith("|%s" % nonsanitize_parameter) for
nonsanitize_parameter in self.NEVER_SANITIZE]):

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

Lines 721-723 should have an additional 4-space indentation.

This comment has been minimized.

Copy link
@dannon

dannon Aug 17, 2017

Member

You think? I actually prefer conditions in a group as listed above, and I thought this was a legit style to use. (that is, all conditions vertically aligned)

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

See https://www.python.org/dev/peps/pep-0008/#indentation : "When the conditional part of an if-statement is long enough to require that it be written across multiple lines, it's worth noting that the combination of a two character keyword (i.e. if), plus a single space, plus an opening parenthesis creates a natural 4-space indent for the subsequent lines of the multiline conditional. This can produce a visual conflict with the indented suite of code nested inside the if-statement, which would also naturally be indented to 4 spaces. This PEP takes no explicit position on how (or whether) to further visually distinguish such conditional lines from the nested suite inside the if-statement."
Examples follow there, I think the 3rd one is the best option to resolve the above problem, but it's not enforced.

This comment has been minimized.

Copy link
@dannon

dannon Aug 17, 2017

Member

Yeah, I've read the pep and see the examples. I still think the vertical alignment is valid (per the pep), easier to grasp the components of the conditional expression at a glance, and reasonable because there's still a visual break prior to the child block.

This comment has been minimized.

Copy link
@jmchilton

jmchilton Aug 17, 2017

Author Member

I'm going to leave this as is given the disagreement and since this PR doesn't actually touch those lines anyway - is that okay with everyone for now? @nsoranzo if you want to open a followup PR that would be the right place to discuss this further I think?

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

@dannon The visual break is there only because of the more-indented list comprehension being at the end of the condition, try to swap the last 2 conditions:

                if (value is not None and
                    True not in [key.endswith("|%s" % nonsanitize_parameter) for
                                 nonsanitize_parameter in self.NEVER_SANITIZE] and
                    key not in self.NEVER_SANITIZE):
                    self.__dict__[key] = sanitize_param(value)

I know it's valid, but if we aim at consistency then the double indentation always works:

                if (value is not None and
                        key not in self.NEVER_SANITIZE and 
                        True not in [key.endswith("|%s" % nonsanitize_parameter) for
                                     nonsanitize_parameter in self.NEVER_SANITIZE]):
                    self.__dict__[key] = sanitize_param(value)

@jmchilton Fine for me.

@@ -715,23 +721,24 @@ def filter(self, trans, user, query, column_filter):

def get_filter(self, trans, user, column_filter):
# Parse filter to extract multiple tags.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

This comment is also over-indented.

@@ -715,23 +721,24 @@ def filter(self, trans, user, query, column_filter):

def get_filter(self, trans, user, column_filter):
# Parse filter to extract multiple tags.
if isinstance(column_filter, list):
if isinstance(column_filter, list):
# Collapse list of tags into a single string; this is redundant but effective. TODO: fix this by iterating over tags.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

This comment is also over-indented.

raw_tags = trans.app.tag_handler.parse_tags(column_filter.encode("utf-8"))
clause_list = []
for name, value in raw_tags:
if name:
# Filter by all tags.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

This comment is also over-indented.

clause_list.append(self.model_class.tags.any(func.lower(self.model_tag_association_class.user_tname).like("%" + name.lower() + "%")))
if value:
clause_list.append(self.model_class.tags.any(func.lower(self.model_tag_association_class.user_tname).like("%" + name.lower() + "%")))
if value:
# Filter by all values.

This comment has been minimized.

Copy link
@nsoranzo

nsoranzo Aug 17, 2017

Member

This comment is also over-indented.

Add autopep8 script to help rebasing branches after #4440.
Fixes a few more PEP8 issues as part of the autopep8'ing as well.

@jmchilton jmchilton force-pushed the jmchilton:autopep8 branch from 753a256 to 0fa287f Aug 17, 2017

@dannon dannon merged commit 68e8abc into galaxyproject:dev Aug 17, 2017

5 of 6 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
api test Build finished. 281 tests run, 0 skipped, 0 failed.
Details
framework test Build finished. 153 tests run, 0 skipped, 0 failed.
Details
integration test Build finished. 44 tests run, 0 skipped, 0 failed.
Details
lgtm analysis: JavaScript No alert changes
Details
toolshed test Build finished. 579 tests run, 0 skipped, 0 failed.
Details
@jmchilton

This comment has been minimized.

Copy link
Member Author

commented Aug 17, 2017

Thanks for the merge @dannon and the fix bug spotting both of you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.