Yep. Also surprised by the 50/50 - so far the "easy" path is blocked by INFRA, so I am not sure if we will quickly do it, but I will likely see what we can do soon.
And yes. This is the same for me - I **LOVE** black and always have pre-commit installed because I do not have to spend any mind-cycles on things that are extremely important for the project and readability (i.e. consistency) but extremely unnecessary to worry about it when I think about solving real problems. On Thu, Jan 4, 2024 at 8:05 PM Oliveira, Niko <oniko...@amazon.com.invalid> wrote: > > Interesting how 50/50 this one has turned out to be! > > I'm personally in favour (+1). The less I have to worry about accidental > typos, indentation, quoting, etc the better, I can focus on important > changes. It will also unblock many PRs from contributors that are otherwise > mergeable except for being stuck on very simple static check failures, which > as a maintainer sounds very nice (it will solve having to post the regular > comment of "please run and fix static checks"). > > And ultimately if the bot does something silly (just as a human can and often > does) we can catch it in the PR review. > > > Cheers, > Niko > > ________________________________ > From: Wei Lee <weilee...@gmail.com> > Sent: Tuesday, January 2, 2024 5:58:18 PM > To: dev@airflow.apache.org > Subject: RE: [EXTERNAL] [COURRIEL EXTERNE] [DISCUSSION] Enabling > `pre-commit.ci` application for Airflow > > CAUTION: This email originated from outside of the organization. Do not click > links or open attachments unless you can confirm the sender and know the > content is safe. > > > > AVERTISSEMENT: Ce courrier électronique provient d’un expéditeur externe. Ne > cliquez sur aucun lien et n’ouvrez aucune pièce jointe si vous ne pouvez pas > confirmer l’identité de l’expéditeur et si vous n’êtes pas certain que le > contenu ne présente aucun risque. > > > > Same as Amogh. Even though I would like to fix that myself, it would make it > much easier for those who aren’t familiar with these tools and still be able > to contribute. But we might need to doc this behavior somewhere (GitHub PR > issue might make more sense 🤔). Otherwise, the contributor might be surprised > by the new commit. > > Best, > Wei > > > On Jan 3, 2024, at 12:21 AM, Vincent Beck <vincb...@apache.org> wrote: > > > > I like the concept! +1 > > > > On 2023/12/30 11:16:35 Amogh Desai wrote: > >> I am aligning here with Pierre, but I am not against the idea of enabling > >> the pre commit ci application. > >> > >> I’d rather have myself fix the issue as it sometimes also lets me have > >> second,third or multiple passes at my code which is up for review. This is > >> a personal choice where I feel that we are trying to fix a problem that is > >> not too problematic. > >> > >> Again, only a personal choice but not against it. If it makes lives of the > >> stakeholders involved easier, I am all up for it! > >> > >> Thanks & Regards, > >> Amogh Desai > >> > >> On Sat, 30 Dec 2023 at 2:35 PM, Pierre Jeambrun <pierrejb...@gmail.com> > >> wrote: > >> > >>> I like the idea, but in practice auto fixable static checks are very > >>> obvious to fix and doesn’t require much work. > >>> > >>> On the other hand most of static failure are ‘real issues’ and not auto > >>> fixable, for instance mypy, spelling, sphinx, db session usage etc…. (And > >>> ruff fix is a little aggressive IMO regarding linting). > >>> > >>> So I would say that in practice it solves a painless problem (formatting, > >>> import sorting and other obvious things) and can’t do much about other > >>> issues. > >>> > >>> This is why I am not sure it is worth the confusion for users. (But I am > >>> not against it) > >>> > >>> On Sat 30 Dec 2023 at 09:19, Scheffler Jens (XC-DX/PJ-PACE-E03) > >>> <jens.scheff...@de.bosch.com.invalid> wrote: > >>> > >>>> I‘d also like to have auto-fixing included in CI. It is a classic pitfall > >>>> and all that can be automated does not need to be a manual burden. > >>>> Though I am not sure whether the plugin is able to use all the custom > >>>> stuff as we also depend during execution on the CI image and docker. > >>>> Besides security things this would be something that needs testing if it > >>>> works. > >>>> > >>>> TLDR: +1 opinion > >>>> > >>>> Sent from Outlook for iOS<https://aka.ms/o0ukef> > >>>> ________________________________ > >>>> From: Pankaj Koti <pankaj.k...@astronomer.io.INVALID> > >>>> Sent: Saturday, December 30, 2023 7:50:10 AM > >>>> To: dev@airflow.apache.org <dev@airflow.apache.org> > >>>> Subject: Re: [DISCUSSION] Enabling `pre-commit.ci` application for > >>> Airflow > >>>> > >>>> I very much like the concept. We have been using it actively for > >>> Astronomer > >>>> code repositories for 1+ year already and it has helped us greatly > >>> (Thanks > >>>> to Felix Uellendall for introducing this back then 🙂) > >>>> > >>>> On Sat, 30 Dec 2023, 12:10 Jarek Potiuk, <ja...@potiuk.com> wrote: > >>>> > >>>>> FYI - Just now INFRA rejected the request on the basis of "code write" > >>>>> access permissions the app needs. > >>>>> > >>>>> I'd still love to get feedback though on the concept - I am not giving > >>>> up > >>>>> that easily. We might still get it approved easily. We likely have some > >>>>> ways we can get "auto-fixing" working for us. > >>>>> > >>>>> 1) I believe Github Applications now can use a bit different mechanism > >>> to > >>>>> ask for permissions and it can have "required" and "optional" > >>> permissions > >>>>> and I believe "Pull request write" should be enough (and I might > >>> attempt > >>>> to > >>>>> convince the maintainers of it to adapt it to our needs). > >>>>> 2) Also, there is a "Pre-commit Lite Github Action" that we **might** > >>> be > >>>>> able to use to achieve a similar effect (with some added complexity to > >>>> our > >>>>> `Pull Request Target` workflow. > >>>>> > >>>>> So I would still love to hear from others :) > >>>>> > >>>>> J. > >>>>> > >>>>> On Fri, Dec 29, 2023 at 11:52 PM Jarek Potiuk <ja...@potiuk.com> > >>> wrote: > >>>>> > >>>>>> Hello everyone, > >>>>>> > >>>>>> TL;DRl; I'd like to propose that we enable the pre-commit-ci GitHub > >>>>>> application for Airflow repo. According to how I understand it works, > >>>> it > >>>>>> should greatly reduce friction (especially for new contributors) for > >>>>>> passing the quality gates for our pre-commits. That is - if we get > >>> the > >>>>>> approval for pre-commit-ci application from the ASF infra team. > >>>>>> > >>>>>> Some more context: > >>>>>> > >>>>>> We use and love (well some of us do, some of us likely hate) the > >>>>>> pre-commit as a quality gate for our checks. We have been using it > >>> for > >>>>>> years for local checks and CI integration and we have ~60 custom > >>>>> precommits > >>>>>> and in total we use about 100 pre-commit checks as our "quality" > >>> gates > >>>>>> > >>>>>> However, using `standard` pre-commit (that is a de-facto standard in > >>>>>> Python world) has a nice property of 'standing on the shoulders of > >>>>> giants'. > >>>>>> There is one thing that few of us are aware of, that there is a way > >>> to > >>>>>> reduce friction for pre-commits that are not only flagging errors but > >>>> can > >>>>>> also fix them. If we get the `pre-commit-ci` application ( > >>>>>> > >>>> > >>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpre-commit.ci%2F&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349294376%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=tgcyFz64S3IBLcAAYUYjswy7cqG%2FZ0KNbgRtSJyxOxQ%3D&reserved=0 > >>>> )<https://pre-commit.ci/> approved for our repo from the ASF infra team > >>> it > >>>>>> - in theory - should be able to AUTO-FIX PRs that are not passing the > >>>>>> pre-commits (and can be automatically corrected). > >>>>>> > >>>>>> Yep. You read it right. No more asking a new contributor "please fix > >>>>>> static checks" - PRs that have auto-fixable pre-commit failures will > >>> be > >>>>>> fixed automatically. > >>>>>> > >>>>>> For example when you make a PR that does not pass "ruff" formatting, > >>>> the > >>>>>> application should automatically amend the PR and FIX IT. We have > >>>> quite a > >>>>>> number of such PRs from first-time contributors, but also a number of > >>>>>> seasoned contributors (including myself) occasionally send a PR that > >>>> does > >>>>>> not pass an auto-fixable static check. This can happen with a few > >>>>> scenarios > >>>>>> (rebasing, or correcting a PR by applying a suggestion from review > >>> and > >>>> a > >>>>>> few other scenarios). > >>>>>> > >>>>>> It can be a little strange to see your PR corrected by a bot though, > >>>> so I > >>>>>> am reaching out here to see if you think it is a good idea. I also > >>>>> opened a > >>>>>> JIRA request to approve the application (but I made a comment that it > >>>>>> should be pending the discussion here): > >>>>>> > >>>> > >>> https://eur03.safelinks.protection.outlook.com/?url=https%3A%2F%2Fissues.apache.org%2Fjira%2Fbrowse%2FINFRA-25322&data=05%7C02%7CJens.Scheffler%40de.bosch.com%7C7292ed56c2374a93eb1508dc09039ec0%7C0ae51e1907c84e4bbb6d648ee58410f4%7C0%7C0%7C638395158349450599%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&sdata=mRyEImOqrqGXmbMxxLvPG5H%2F5J5CsnNdZH%2FYjJeRJLg%3D&reserved=0 > >>>> <https://issues.apache.org/jira/browse/INFRA-25322> - it will likely > >>>>>> require to slightly change our workflows to make it works as well. > >>>>>> > >>>>>> Do you think it's a good idea to have it enabled? Maybe it will be > >>> too > >>>>>> much for our contributors and they will be surprised to see it > >>>> happening? > >>>>>> > >>>>>> WDYT? > >>>>>> > >>>>>> J. > >>>>>> > >>>>> > >>>> > >>> > >> > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > > For additional commands, e-mail: dev-h...@airflow.apache.org > > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org > For additional commands, e-mail: dev-h...@airflow.apache.org > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org For additional commands, e-mail: dev-h...@airflow.apache.org
