Vote +1 for this, I do. (Sorry couldn't help it) It's nice that there is some automation to do this. It was always slightly annoying to me coming from a different programming language background
Thanks & Regards, Amogh Desai On Fri, Nov 29, 2024 at 12:00 PM Shahar Epstein <sha...@apache.org> wrote: > +1 Agree, I do :-) > > On Thu, Nov 28, 2024 at 9:12 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. > > >
