Yeah this seems pretty uncontroversial. I assume most of the occurrences are left over from when we converted to pytest. I tend to fix them as I go, since pytest expects it this way. Did not know there was automation for it but that’s great.
On Thu, Nov 28, 2024 at 6:06 PM Wei Lee <weilee...@gmail.com> wrote: > +1 for this! > > This is how pytest doc > https://docs.pytest.org/en/stable/how-to/assert.html suggests. > > Best, > Wei > > > On Nov 29, 2024, at 8:28 AM, Kaxil Naik <kaxiln...@gmail.com> wrote: > > > > Same as slack, prefer it on right. Right is right 😆 > > > > On Thu, 28 Nov 2024 at 23:48, Hussein Awala <huss...@awala.fr> wrote: > > > >> +1 > >> > >> On Fri, Nov 29, 2024 at 12:28 AM Jarek Potiuk <ja...@potiuk.com> wrote: > >> > >>> FYI: This is how it will look like when SIM300 is applied: > >>> https://github.com/apache/airflow/pull/44466 > >>> > >>> On Thu, Nov 28, 2024 at 11:23 PM Jens Scheffler > >> <j_scheff...@gmx.de.invalid > >>>> > >>> wrote: > >>> > >>>> This is nto a vote but still +1 :-D > >>>> > >>>> On 28.11.24 22:48, Ash Berlin-Taylor wrote: > >>>>> +10, it’s defence against a problem that is impossible in Python, > >> _and_ > >>>> it’s counter to how pytest wants it. > >>>>> > >>>>>> On 28 Nov 2024, at 21:46, Ferruzzi, Dennis > >>> <ferru...@amazon.com.INVALID> > >>>> wrote: > >>>>>> > >>>>>> +0.5 LOL I don't particularly care which order they go in, but I > >>> do > >>>> love standardizing the codebase and that IS how pytest reports it, so > I > >>> say > >>>> let's go for it. > >>>>>> > >>>>>> > >>>>>> - ferruzzi > >>>>>> > >>>>>> > >>>>>> ________________________________ > >>>>>> From: Jarek Potiuk <ja...@potiuk.com> > >>>>>> Sent: Thursday, November 28, 2024 11:16 AM > >>>>>> To: dev@airflow.apache.org > >>>>>> Subject: RE: [EXT] [DISCUSS] Avoid Yoda conditions in our code > >>>>>> > >>>>>> 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. > >>>>>> > >>>>>> > >>>>>> > >>>>>> OK. And now let the flamewar starts ;) > >>>>>> > >>>>>> On Thu, Nov 28, 2024 at 8:11 PM Jarek Potiuk <ja...@potiuk.com> > >>> wrote: > >>>>>> > >>>>>>> Hello here, > >>>>>>> > >>>>>>> Following from slack discussion: > >>>>>>> > >>>> > >> https://apache-airflow.slack.com/archives/C06K9Q5G2UA/p1732816777850809 > >>>>>>> > >>>>>>> While responding to > >>>> https://github.com/apache/airflow/discussions/44455 > >>>>>>> (really nice that Astral people are using Airflow to test new ruff > >>>> rules) I > >>>>>>> noticed that we have a number of cases where we use > >>>>>>> > >>>>>>> assert EXPECTED_CONSTANT == value tested > >>>>>>> > >>>>>>> rather than > >>>>>>> > >>>>>>> assert value tested == EXPECTED_CONSTANT > >>>>>>> > >>>>>>> I just learned that The way to write this condition is called "Yoda > >>>>>>> condition" :) - for obvious reasons :) > >>>>>>> > >>>>>>> I know where it comes from - > >>>>>>> https://wiki.c2.com/?CompareConstantsFromTheLeft - because there > >> is > >>>> (was) > >>>>>>> easy to make mistake and have `=` instead of `==`, but this is > >> from C > >>>> and > >>>>>>> not from Python. In Python it literally makes no sense to prevent > >>> this, > >>>>>>> because it leads to a syntax error: > >>>>>>> > >>>>>>>>>> x = 10 > >>>>>>>>>> assert x = 9 > >>>>>>> File "<stdin>", line 1 > >>>>>>> assert x = 9 > >>>>>>> ^ > >>>>>>> SyntaxError: invalid syntax > >>>>>>> Also Pytest is very clear in their "assert rewritten messages" that > >>> the > >>>>>>> "right" side is expected > >>>>>>> > >>>>>>> tests/jobs/test_scheduler_job.py:1695 > >>>>>>> > >> (TestSchedulerJob.test_execute_task_instances_limit_second_executor) > >>>>>>> 7 != 6 > >>>>>>> > >>>>>>> Expected :6 > >>>>>>> Actual :7 > >>>>>>> <Click to see difference> > >>>>>>> > >>>>>>> test_scheduler_job.py:1736: in > >>>>>>> test_execute_task_instances_limit_second_executor > >>>>>>> assert 7 == res > >>>>>>> E assert 7 == 6 > >>>>>>> > >>>>>>> IMHO - using this `assert CONSTANT == value` - is a bit of a "cargo > >>>> cult" > >>>>>>> - we are using it without realising that it is no use any more and > >>> only > >>>>>>> introduces confusion. > >>>>>>> > >>>>>>> Quick check shows that we have 398 "yoda asserts" in our tests, so > >>>>>>> apparently the cult is pretty strong :D. > >>>>>>> > >>>>>>> I would say we should make Yoda conditions forbidden via the > >>>>>>> https://docs.astral.sh/ruff/rules/yoda-conditions/ rule and fix > >> all > >>>> those > >>>>>>> Yoda conditions to be "non-Yoda" conditions (including all the > >>>> asserts). > >>>>>>> > >>>>>>> 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 > >>>> > >>>> > >>> > >> > >
