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

Uniform indentation of multiline if conditionals #4455

Merged
merged 2 commits into from Aug 18, 2017

Conversation

Projects
None yet
2 participants
@nsoranzo
Copy link
Member

commented Aug 18, 2017

Use 8-spaces indentation for the conditional continuation lines of if-statements where the conditional part spans multiple lines.

According to PEP-8, using 4-spaces "can produce a visual conflict with the indented suite of code nested inside the if-statement", but the PEP "takes no explicit position".

There were only 2 places in the whole codebase where 8 spaces were not used, this will make it uniform.

nsoranzo added some commits Aug 17, 2017

Uniform indentation of multiline if conditionals
Use 8-spaces indentation for the conditional continuation lines of an
`if`-statement where the conditional part spans multiple lines.

According to PEP-8, using 4-spaces "can produce a visual conflict with
the indented suite of code nested inside the `if`-statement", but the PEP
"takes no explicit position".

There were only 2 places in the whole codebase where 8 spaces were not
used, this will make it uniform.

@nsoranzo nsoranzo requested a review from dannon Aug 18, 2017

@nsoranzo nsoranzo added this to the 17.09 milestone Aug 18, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

I think the first one you fixed here is less readable, vertical alignment is more important to me. In my opinion, the best fix is extra indentation in child blocks when this is a problem, not offsetting the vertical alignment of the conditionals.

That said, I feel like I'm in the minority here, it's not something I'd -1 anyway, and you bundled all those other changes, too, so, merging.

@dannon dannon merged commit 9c6fd67 into galaxyproject:dev Aug 18, 2017

6 checks passed

api test Build finished. 281 tests run, 0 skipped, 0 failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
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
@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Thanks @dannon!

@nsoranzo nsoranzo deleted the nsoranzo:multiline_if_conditionals branch Aug 18, 2017

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

(not that I'm encouraging bundling fixes just to get things through! ;))

@nsoranzo

This comment has been minimized.

Copy link
Member Author

commented Aug 18, 2017

Sorry, that was not my intention, I just have the habit of doing my 2to3 process and import fixes on files I touch to slowly advance #1715.

@dannon

This comment has been minimized.

Copy link
Member

commented Aug 18, 2017

@nsoranzo Wasn't saying it was. I was saying that I merged this because I'm -0 on that particular change, but appreciate the others, and then I realized I might need to clarify the bundling remark for posterity, and I think that came across wrong. Thanks again!

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.