+1 agree :) Regards, Pavan
On Fri, Nov 29, 2024, 06:37 Amogh Desai <amoghdesai....@gmail.com> wrote: > 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. > > > > > >
