[ https://issues.apache.org/jira/browse/BEAM-9274?focusedWorklogId=385424&page=com.atlassian.jira.plugin.system.issuetabpanels:worklog-tabpanel#worklog-385424 ]
ASF GitHub Bot logged work on BEAM-9274: ---------------------------------------- Author: ASF GitHub Bot Created on: 11/Feb/20 20:15 Start Date: 11/Feb/20 20:15 Worklog Time Spent: 10m Work Description: chadrik commented on pull request #10810: [BEAM-9274] Support running yapf in a git pre-commit hook URL: https://github.com/apache/beam/pull/10810#discussion_r377873324 ########## File path: .pre-commit-config.yaml ########## @@ -0,0 +1,32 @@ +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +repos: + - repo: https://github.com/pre-commit/mirrors-yapf + # this rev is a release tag in the repo above and corresponds with a yapf + # version. make sure this matches the version of yapf in tox.ini. + rev: v0.29.0 + hooks: + - id: yapf + files: ^sdks/python/apache_beam/ + # keep these in sync with sdks/python/.yapfignore Review comment: First, some background: pre-commit triggers its hooks based on changed files, matching based on 3 values: - file type (e.g. it has a mapping from "python" to , ".py", etc) - files: include patttern - exclude: exclude pattern pre-commit always passes an explicit list of changed files to the underlying tool, and never relies on recursive flags. Ok, on to your question. Our exclude patterns cover autogenerated files. So, if a new autogenerated file were added to the repo which was excluded in .yapfignore but not in .pre-commit-config.yaml (which I think would be the most common "failure" scenario), then the first time that a developer with pre-commit enabled changed that file (say, by regenerating it) and tried to commit those changes, yapf would autoformat it, and then pre-commit would fail the commit with a message like this: ``` yapf.....................................................................Failed - hook id: yapf - files were modified by this hook ``` At this point hopefully the developer realizes that the failure is due to yapf, and finds the trail of comments leading them to .pre-commit-config.yaml. Worst case scenario is that they commit it with the autoformatted changes, which would be pretty innocuous, and would not fail any jenkins tests because those same autogenerated files are excluded from pylint. Another thing to note is that this is all just a convenience for developers. Jenkins remains our last line of defense. Also note that it's possible to invoke your lint tools using pre-commit within tox, so that you can consolidate all of your includes and excludes across all tools into one file, the pre-commit-config.yaml. This is what we do where I work. ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org Issue Time Tracking ------------------- Worklog Id: (was: 385424) Time Spent: 1h (was: 50m) > Support running yapf in a git pre-commit hook > --------------------------------------------- > > Key: BEAM-9274 > URL: https://issues.apache.org/jira/browse/BEAM-9274 > Project: Beam > Issue Type: Improvement > Components: sdk-py-core > Reporter: Chad Dombrova > Assignee: Chad Dombrova > Priority: Major > Time Spent: 1h > Remaining Estimate: 0h > > As a developer I want to be able to automatically run yapf before I make a > commit so that I don't waste time with failures on jenkins. -- This message was sent by Atlassian Jira (v8.3.4#803005)