Sounds good to me On Fri, Jan 21, 2022 at 5:17 PM Jarek Potiuk <[email protected]> wrote:
> Hello everyone, > > We are very quickly approaching the "0 MyPy errors" state. This was a > truly collaborative effort of many people :). Fantastic job ! > > Now - I will shortly (when we reach 0!) re-enable MyPy as "failing" tests, > but this might have a rather nasty side effect that some of the PRs > approved and not rebased before might introduce new errors which will break > main (and other PRs). > I would like to mitigate it by introducing - for a couple of days/week a > rule that all PRs should be rebased to main before merging. We already have > a check for that (but it is enabled only in case of "sqlalchemy migration" > changes). But I would like to enable it for any change that involves python > code changes. We would run it for a few days and only enable MyPY as > "obligatory" then and keep it enabled for a few days after. Also I think - > independently - committers should pay attention when migrating older PRs > and ask the authors to rebase (but this is not easily visible as you really > need to check status of the branch PR is coming from to see if the PR is > rebased). > > Do you think those are good ideas? > > J. > > > On Wed, Dec 1, 2021 at 9:59 AM Jarek Potiuk <[email protected]> wrote: > >> Thanks Khailid (and Josh who commented on the issue) for volunteering to >> help! >> >> And I think we need more helping hands! >> >> I have a very kind request to the whole community to help with the >> effort as we have whooping 990 (!) issues to fix. Without the help of >> the community it will take a few weeks at least, but with lots of >> helping hands we might do it much faster and get much stronger quality >> protections in place. >> >> I merged a change and tested it on a few PRs >> (https://github.com/apache/airflow/pull/19914, >> https://github.com/apache/airflow/pull/19912, >> https://github.com/apache/airflow/pull/19911) to see how this can work >> on it. >> >> We have now a very simple incremental way of fixing mypy that anyone >> can contribute to - either as separate PRs or as part of their usual >> contributions. >> >> In the issue https://github.com/apache/airflow/issues/19891 we keep an >> inventory of packages that need fixing (and I will keep it updated). >> >> How to help ? >> >> VERY EASY: >> >> * Checkout latest main. >> * Run `./breeze build-image` and rebuild the image (you need it to get >> latest mypy and related libraries included) >> * `pip install pre-commit` >> * `pre-commit install >> >> That's all that is needed as prerequisites. >> >> There are two ways to help: >> >> 1) If you make a regular contribution - whenever you commit your code, >> mypy will fail with the issues that need fixing. It's as simple as >> that. Just work as normal and make your pre-commit pass. >> 2) If you want to take on correcting the whole package as a separate >> PR, just commend in the https://github.com/apache/airflow/issues/19891 >> that you are working on it and then you can do this: >> >> `find <DIRECTORY> -name "*.py" | xargs pre-commit run mypy --files` >> >> It will show you what needs to be fixed. >> >> One comment from TP who started commenting on the PRs (and I very much >> agree to) - rather than slapping '# type ignore` everywhere, we should >> aim to really bring stronger type checking - even if it means slight >> refactors, or changing the structure of the code. a bit. The aim is >> not to "pass" mypy, but to improve the quality of our code. >> >> Thanks in advance for cooperation! >> >> J. >> >> >> J. >> >> On Tue, Nov 30, 2021 at 3:20 PM Khalid Mammadov >> <[email protected]> wrote: >> > >> > Hi Jarek, >> > >> > >> > Sounds great! >> > >> > I can also help to look into these new issues and fix. >> > >> > >> > Regards, >> > >> > Khalid >> > >> > On 30/11/2021 12:14, Jarek Potiuk wrote: >> > > Hey all, >> > > >> > > I created an issue for that here: >> > > https://github.com/apache/airflow/issues/19891 but wanted to get a >> > > lazy consensus on that one (I think it does not really need voting or >> > > consensus, but If there is anyone who wants to comment/object fill >> > > free). >> > > >> > > For a few weeks MyPy checks have been disabled after the switch to >> > > Python 3.7 (per https://github.com/apache/airflow/pull/19317). >> > > >> > > We should, however, re-enable it back as it is very useful in catching >> > > a number of mistakes. >> > > >> > > I am re-adding the mypy pre-commit now - with mypy bumped to 0.910. >> > > This version detects far more errors and we should fix them all before >> > > we switch the CI check back. >> > > >> > > The way I am bringing it back in >> https://github.com/apache/airflow/pull/19890: >> > > >> > > * mypy will be running for incremental changes in pre-commit, same as >> > > before. This will enable incremental fixes of the code changed by >> > > committers who use pre-commits locally >> > > >> > > * mypy on CI runs in non-failing mode. When the main pre-commit check >> > > is run, mypy is disabled, but then it is run as a separate step (which >> > > does not fail but will show the result of running mypy on all our >> > > code). This will enable us to track the progress of fixes >> > > >> > > We should make a concerted effort now and incrementally fix all the >> > > mypy incompatibilities - ideally package/by/package to avoid huge code >> > > reviews. >> > > >> > > I'd really appreciate a number of people to contribute, so that we can >> > > re-enable mypy back fully :). >> > > >> > > J. >> >
