You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by GitBox <gi...@apache.org> on 2020/05/04 16:38:25 UTC
[GitHub] [airflow] tedmiston opened a new pull request #8710: Make list trailing comma usage consistent
tedmiston opened a new pull request #8710:
URL: https://github.com/apache/airflow/pull/8710
I noticed a few places in the codebase were trailing list commas were used inconsistently. This PR is an enhancement to improve the consistency of list trailing comma usage.
More specifically this PR:
1. Updates several single-item lists that had a redundant trailing comma (meaningful for tuples but not for lists).
1. Updates some lists that had a redundant trailing comma on the same line as the closing bracket.
1. Updates a few instances were there was no trailing comma for lists where the closing bracket was on a new line.
---
Make sure to mark the boxes below before creating PR: [x]
- [x] Description above provides context of the change
- [x] Unit tests coverage for changes (not needed for documentation changes) - n/a
- [x] Target Github ISSUE in description if exists - n/a
- [x] Commits follow "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)"
- [x] Relevant documentation is updated including usage instructions. - n/a
- [x] I will engage committers as explained in [Contribution Workflow Example](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#contribution-workflow-example).
---
In case of fundamental code change, Airflow Improvement Proposal ([AIP](https://cwiki.apache.org/confluence/display/AIRFLOW/Airflow+Improvements+Proposals)) is needed.
In case of a new dependency, check compliance with the [ASF 3rd Party License Policy](https://www.apache.org/legal/resolved.html#category-x).
In case of backwards incompatible changes please leave a note in [UPDATING.md](https://github.com/apache/airflow/blob/master/UPDATING.md).
Read the [Pull Request Guidelines](https://github.com/apache/airflow/blob/master/CONTRIBUTING.rst#pull-request-guidelines) for more information.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-624354898
This branch is now freshly rebased off of master.
I've also updated the MD5 checksums for the two Breeze CI Requirements checks that were failing. [Note that my only change to setup.py was only stylistic, not functional, so there is no change to actual dependencies for this PR.] All CI checks should be passing now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-623633477
How do you want to prevent regression?Without automatic checks, it is very difficult to keep one style in the OSS project.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
potiuk commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-636887320
@tedmiston - I do appreciate intentions, and I perfectly understand your thinking. That would also be my natural instinct what you described and I did it in the past in Airflow - and got burned several times by this myself (as I am often doing cherry-picking to 1.10). While I sympathize with all you said, I have to look at this from my position as well (and other people who cherry-pick).
First of all - I think making such consistency change should always (no exception) be accompanied with an automated enforcing of the rule introduction. Otherwise, it is like continuous dust cleaning that you have to repeat endlessly. I think (as @mik-laj already mentioned we already agreed that we will introduce consistent formatting at some point in the future. And we cannot introduce it now - precisely because we are cherry-picking.
We are automating all our syntax rules relentlessly (with pylint, flake8, and mypy and it's not yet fully complete) and I just do not feel that it's worth doing this kind of change now. It's better to work on the remaining todo list of pylint and fix all the remaining code to pass pylint checks.
Eventually, we will choose (likely Black) one consistent formatter for everything and forget about it, but we do not really want to do it for the sake of doing now - while doing heavy cherry-picking. It would be acceptable if it's part of another change and the files were touched for other reasons (refactoring) but not when it's added on its own. It will only add noise to our cherry-picking effort as @mik-laj mentioned. I'd simply hold-off with this kind of change. Just a little patience and we get there. Things get a bit slower when your codebase grows as much as ours.
But we are getting there - we already added (and continue working on) consistency of names and packages, removing and unentangling some internal dependencies in the core, etc. etc. We are getting there.
Sorry for the push-back this time, It's hard for me to say no to introducing more consistency but those are the circumstances we are at now.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-624398918
I totally understand reducing the amount of extra lift with backporting to v1.10. I have contributed PRs to Airflow pre-2.0, so it totally makes sense to me.
I have also contributed multiple stylistic improvement PRs to Airflow in the past and had them merged already.
I'm confused by the response here — What's the argument against improving the convention in small meaningful ways now that help us better maintain both versions? I do not see a downside to this PR.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-624360039
We also have AIP about code-formatting: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=99844429
This has not been completed for many reasons, but the most important is making it difficult to cherry-pick changes to 1.10. Two Airflow series are currently being developed: 1.10 and 2.0 (master). All changes are developed in Airflow 2.0, but then the selected changes are manually cherry-picked to Airflow 1.10 by a small group of project committers. This is an increasingly difficult task because Airflow 2.0 only supports Python 3+ and Airflow 1.10 supports Python 2 and 3. For this reason, we try not to accept changes that introduce mass changes in many files for no good reason.
If you want this change to be accepted, you must provide strong arguments to support this change.
Here is another discussion: https://github.com/apache/airflow/pull/7573
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-623655524
@mik-laj I agree it's difficult to keep consistent styles longterm without automatic checking.
When I'm working on a new PR changing code that doesn't have an automatic format check, I look around at the code I'm modifying to ensure the style is consistent. My thought process in this PR is that improving baseline consistency is better than not, at least for others who look at nearby style when authoring (so that they see one style instead of multiple).
As far as making the process automatic, I have had good results with [Black][1] (PSF). It can also be conveniently added to CI / pre-commit hooks (`black --diff ...`). The [Prefect][3] project along with many others already use Black today.
I've also used [YAPF][2] (Google) in the past. It's more mature and much more configurable than Black. In that sense it is much more like ESLint vs Black being more opinionated like gofmt.
I have had good experiences with both, so I don't have a strong preference for one over the other necessarily, but I do think adding a code formatter to Airflow would benefit everyone today (especially given how much the codebase has grown over the past couple years). What are your thoughts?
[1]: https://github.com/psf/black
[2]: https://github.com/google/yapf
[3]: https://github.com/PrefectHQ/prefect
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-624356451
Update on using an auto formatter for style: I did some searching and saw that @Fokko also mentioned adopting similar options back in https://github.com/apache/airflow/pull/3772.
Do you think that type of change is substantial enough that it warrants an AIP?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-631525571
Hello reviewers - please find this PR has been updated with a fresh rebase off master to resolve conflicts with the updated requirements md5 files.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] Fokko commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
Fokko commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-637009002
There is an [AIP already](https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=99844429) (more than a year ago). For me, it feels like we're being held back by Airflow 1.x. Cherry-picking stuff back is black magic already and has also been proven very error-prone. It would be great if we (eventually) could apply Black to the master branch, which would make the code much more readable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj edited a comment on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj edited a comment on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-623633477
How do you want to prevent regression?Without automatic checks, it is very difficult to watch the style in the OSS project.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-636846627
Hi @mik-laj can you reply to recent comments?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] tedmiston commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
tedmiston commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-631626472
It looks like the Travis build is failing on master right now hence the failure here as well (unrelated to these changes).
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-623633477
How do you want to prevent regression?
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-624360039
We also have AIP about code-formatting: https://cwiki.apache.org/confluence/pages/viewpage.action?pageId=99844429
This has not been completed for many reasons, but the most important is making it difficult to cherry-pick changes to 1.10. Two Airflow series are currently being developed: 1.10 and 2.0 (master). All changes are developed in Airflow 2.0, but then the selected changes are manually cherry-picked to Airflow 1.10 by a small group of project committers. This is an increasingly difficult task because Airflow 2.0 only supports Python 3+ and Airflow 1.10 supports Python 2 and 3. For this reason, we try not to accept changes that introduce mass changes in many files for no good reason.
Here is other discussion: https://github.com/apache/airflow/pull/7573
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-636851283
You can use the activity graph when choosing a reviewer. Just choose one person at random and ping them.
https://github.com/apache/airflow/pulse
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] mik-laj commented on pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
mik-laj commented on pull request #8710:
URL: https://github.com/apache/airflow/pull/8710#issuecomment-636850131
Now we have a lot of differences between Airflow 1.10 and 2.0, so even more every conflict is even more painful. However, I am not a release manager, so I do not want to take the final vote. Please treat my opinions as 0. Neither negative nor positive vote.
https://www.apache.org/foundation/voting.html#votes-on-code-modification
If you want this change to be accepted then you need to find another reviewer who will have a different opinion. I just shared my concerns about this change.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [airflow] potiuk closed pull request #8710: Make list trailing comma usage consistent
Posted by GitBox <gi...@apache.org>.
potiuk closed pull request #8710:
URL: https://github.com/apache/airflow/pull/8710
----------------------------------------------------------------
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:
users@infra.apache.org