Merged. On Fri, Nov 29, 2024 at 8:31 AM Jarek Potiuk <ja...@potiuk.com> wrote:
> Cool :). Moar fixes :) > > On Fri, Nov 29, 2024 at 8:13 AM Pavankumar Gopidesu < > gopidesupa...@gmail.com> wrote: > >> Yes, I agree. >> >> I just looked at the ruff , I think we have more to fix in the coming >> days :) >> >> https://github.com/astral-sh/ruff/pull/14661 >> >> https://github.com/astral-sh/ruff/pull/14611 >> >> On Fri, Nov 29, 2024 at 7:06 AM Jarek Potiuk <ja...@potiuk.com> wrote: >> > >> > Seeing the universal support - I progress with >> > https://github.com/apache/airflow/pull/44466 -> Note it's not only for >> > asserts, there are a number of other places where comparison was not >> RIGHT >> > (pun intended).. >> > >> > >> > On Fri, Nov 29, 2024 at 7:50 AM Pavankumar Gopidesu < >> gopidesupa...@gmail.com> >> > wrote: >> > >> > > +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. >> > > > > > >> > > > > >> > > > >> > > >> >> --------------------------------------------------------------------- >> To unsubscribe, e-mail: dev-unsubscr...@airflow.apache.org >> For additional commands, e-mail: dev-h...@airflow.apache.org >> >>
