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

Added YAPF Code Formatting #232

Closed
wants to merge 6 commits into from
Closed

Conversation

ismacaulay
Copy link
Member

@ismacaulay ismacaulay commented Apr 6, 2018

Added Yet Another Python Formatter along with a pre-commit hook to help ensure consistent styling. This will help contributors meet the repository's expected styling. It will be enforced in the travis build just like pylint. See @bsmithyman comment below for more info on styling.

@ismacaulay ismacaulay requested a review from fwkoch April 6, 2018 15:15
@codecov
Copy link

codecov bot commented Apr 6, 2018

Codecov Report

Merging #232 into dev will not change coverage.
The diff coverage is 97.36%.

Impacted file tree graph

@@           Coverage Diff           @@
##              dev     #232   +/-   ##
=======================================
  Coverage   95.44%   95.44%           
=======================================
  Files          17       17           
  Lines        2259     2259           
=======================================
  Hits         2156     2156           
  Misses        103      103
Impacted Files Coverage Δ
properties/images.py 100% <ø> (ø) ⬆️
properties/handlers.py 98.93% <100%> (ø) ⬆️
properties/task/image.py 60.86% <100%> (ø) ⬆️
properties/math.py 98.54% <100%> (ø) ⬆️
properties/link.py 97.87% <100%> (ø) ⬆️
properties/base/__init__.py 100% <100%> (ø) ⬆️
properties/extras/singleton.py 97.36% <100%> (ø) ⬆️
properties/task/base.py 100% <100%> (ø) ⬆️
properties/task/__init__.py 81.81% <100%> (ø) ⬆️
properties/utils.py 96.55% <100%> (ø) ⬆️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cad580c...a343f80. Read the comment docs.

@bsmithyman
Copy link
Member

I haven't gone through line-by-line, but overall this looks pretty clean / consistent. I saw that you put some disables in there where needed – thanks for the extra effort @ismacaulay.

@bsmithyman
Copy link
Member

To ensure consistent code style, we would like to use yapf, which is an unofficial Google project; Yet Another Python Formatter. We use a custom style that closely replicates our target styling for Python source code.

[style]
based_on_style = pep8
dedent_closing_brackets = true
split_complex_comprehension = true
split_before_logical_operator = true
split_before_expression_after_opening_paren = true
split_before_first_argument = true
split_before_named_assigns = true
each_dict_entry_on_separate_line = true
continuation_align_style = fixed
continuation_indent_width = 4
column_limit: 79
allow_multiline_dictionary_keys = false
allow_split_before_dict_value = false

This style is written into a local .style.yapf file in the repository. To install the development requirement, add yapf to the requirements_dev.txt file. To install in your working environment, type pip install yapf. To run, type yapf -rd ... where ... is the working directory (-rd means "recursive, show diffs"). The preceding is the correct call to use in CI tests. The yapf tool also works great with a variety of editor environments.

You can locally disable the formatter by surrounding a block of code in # yapf: disable ... # yapf: enable comments. Please use this selectively, and be prepared to review/reassess in the future if the enclosed code changes.

To automatically style code whenever you commit, create a file called .pre-commit-config.yaml in your repo with the following:

repos:
-   repo: https://github.com/pre-commit/mirrors-yapf
    sha: 'bb280692bd3b1e1f7fab174635ef9678f2452522'
    hooks:
    -   id: yapf

Install with:

pip install pre-commit
pre-commit install

Now you will automatically get styled code when you commit.

@ismacaulay ismacaulay changed the title Cld 3780/code formatting Added YAPF Code Formatting Apr 6, 2018
@ismacaulay ismacaulay changed the base branch from master to dev April 6, 2018 16:08
@fwkoch
Copy link
Contributor

fwkoch commented Apr 12, 2018

Thanks @ismacaulay - I think we will hold off on this for now, though. I'm worried about discouraging contributions by requiring either strict style or an installed pre-commit hook. Also branches properties are held to somewhat strict pylint standards (but in light of this comment, I plan to start only enforcing pylint on deployed versions).

I tagged this work in case we decide to revisit: https://github.com/seequent/properties/tree/yapf-styling

@ismacaulay
Copy link
Member Author

Those are fair concerns, and formatting can be revisited later if we so choose.

@ismacaulay ismacaulay closed this Apr 12, 2018
@ismacaulay ismacaulay deleted the CLD-3780/code-formatting branch April 12, 2018 21:50
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