I'm in favor of this. I love making docs changes directly in GitHub, but I often make a tiny mistake like a trailing space and the tests fail. I think things like this discourage new contributors, as contributing to docs is the easiest way to start getting involved.
On Thu, Jan 4, 2024 at 12:16 PM Jarek Potiuk <[email protected]> wrote: > 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 > <[email protected]> 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 <[email protected]> > > Sent: Tuesday, January 2, 2024 5:58:18 PM > > To: [email protected] > > 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 <[email protected]> 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 < > [email protected]> > > >> 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) > > >>> <[email protected]> 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 <[email protected]> > > >>>> Sent: Saturday, December 30, 2023 7:50:10 AM > > >>>> To: [email protected] <[email protected]> > > >>>> 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, <[email protected]> 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 <[email protected]> > > >>> 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: [email protected] > > > For additional commands, e-mail: [email protected] > > > > > > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: [email protected] > > For additional commands, e-mail: [email protected] > > > > --------------------------------------------------------------------- > To unsubscribe, e-mail: [email protected] > For additional commands, e-mail: [email protected] > >
