Make "deprecated" Note a standard Error, disabled by default by svalentin · Pull Request #18192 · python/mypy (original) (raw)

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation15 Commits3 Checks19 Files changed

Conversation

This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters

[ Show hidden characters]({{ revealButtonHref }})

svalentin

While working on the relase of mypy 1.14 we noticed a large number of notes for deprecated. Speaking with Jukka, he suggested we make this disabled by default. And if it's disabled by default, having it as a note is not as useful. We also don't have many stand alone notes. Most notes are "attached" with an error.

This PR makes the deprecated error disabled by default, removes the flag to report it as error and always reports it as error when enabled.

CC @tyralla

@svalentin

While working on the relase of mypy 1.14 we noticed a large number of notes for deprecated. Speaking with Jukka, he suggested we make this disabled by default. And if it's disabled by default, having it as a note is not as useful. We also don't have many stand alone notes. Most notes are "attached" with an error.

This PR makes the deprecated error disabled by default, removes the flag to report it as error and always reports it as error when enabled.

@github-actions GitHub Actions

This comment has been minimized.

tyralla

``from mod import depr`` statement or uses a deprecated feature imported otherwise or defined
locally. Features are considered deprecated when decorated with ``warnings.deprecated``, as
specified in `PEP 702 https://peps.python.org/pep-0702`_. You can silence single notes via
specified in `PEP 702 https://peps.python.org/pep-0702`_. You can silence single errors via
``# type: ignore[deprecated]`` or turn off this check completely via ``--disable-error-code=deprecated``.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The "or turn off this check..." part of the sentence could also be removed.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! If we decide to go through with this change, I'll take another look at this.

@tyralla

@JelleZijlstra

Keeping this disabled by default feels unfortunate to me; that will make it much harder for users to discover deprecations. However, I understand that turning it on by default is disruptive. Maybe we could turn it on by default in mypy 2.

@ilevkivskyi

Since a note doesn't change the exit code, I think having them on by default is better. I guess it is only annoying for (very) large code bases, but even then it is a simple config change to disable it globally. I don't want to prioritize the convenience of few "larger" users over convenience for many regular users.

@JukkaL

The main reason I proposed this change is that on master mypy generates notes about some pretty popular stdlib functions that are still available in the most recent Python release. For example, GitHub search for the deprecated function datetime.utcnow produces ~500k matches (link). I was just today playing around with some smallish code fragments, and one of them happened to include calls to datetime.utcnow.

Even though the build still passes even if some deprecation notes are shown, extra notes shown on each mypy run are annoying, so effectively every user that gets a note needs to perform some action (disable the error code or remove the uses of deprecated features). This is why I'd consider turning the notes on by default a breaking change, so it should arguably be behind a feature flag, based on our release process. We can consider switching the default in mypy 2.0, which should be coming in the not-too-distant future.

@cdce8p

Speaking for my experience with Home Assistant, I didn't even notice the deprecated notes at first. The full mypy run is usually only done in CI and as long as that was green, I didn't look at the full log. Once I saw them though, my first action was to turn on report_deprecated_as_error.

Thus I'd be in favor of turning it into an error. Personally I wouldn't necessarily consider it a breaking change to even enable it by default as it would be quite simple to disable globally. The breaking change would be if that option to disable it is removed so users are forced to update their code or ignore the issue.

However, if that's the policy, maybe we could target a 2.0 release for Q1 next year and enable it then?

@ilevkivskyi

GitHub search for the deprecated function datetime.utcnow produces ~500k matches

TBH this doesn't really tells much to me. Maybe those people are using a version of Python where it is not deprecated, and/or maybe they don't use mypy, and/or (most likely) that is simply some long abandoned code. The real indicator for me is mypy_primer. Although it is relatively large, it is comparable in size to other things we recently released (for example union/join PRs that caused ~same amount of errors, not notes), so I don't understand what all the panic is about.

Moreover, the current behavior matches other modern tools that emit warnings by default on deprecation (e.g. Rust), with an option to turn warnings into errors.

On a more philosophical level, Python community used to be not very good with deprecations. And IMO this is not because people are lazy, but because until now there were little visibility into deprecations. So again, we should not delay/hide something that is beneficial for majority, because few people may find some notes in the output annoying.

@JukkaL

The real indicator for me is mypy_primer

I got worried about the change since our big internal codebase at work generated thousands of deprecation notes. The volume of output is much more than from any recent mypy change I can remember. Thousands of notes would make mypy unusable without turning the note off or ignoring all the notes (fixing thousands of issues would take a lot of effort).

The above is only one data point, but it might be representative of large proprietary codebases (which mypy primer doesn't cover), so I think we should be careful and introduce the feature as opt-in by default.

However, since this is easy to turn off and for most users the impact seems minor, we can turn it on by default in the next mypy feature release after the initial release, without having to wait for mypy 2.0.

I also think we should generate errors by default, due to these reasons:

So here's my proposal:

I hope the above is an acceptable compromise. We'd have the feature enabled pretty soon (hopefully in a January release), since we don't need to wait for mypy 2.0.

@TeamSpen210

Would it be a good idea to add a config option to allow disabling deprecation warnings for warnings about specific fully qualified names? That might help alleviate lots of errors in cases where you need to use the deprecated feature for some reason. For example I've got deprecations in my own library, but then Mypy flags all the test code and imports which isn't particularly useful. But I want to be warned about deprecations in any dependencies. It'd also allow you to keep CI green, while you locally turn it back on for specific functions to migrate.

@svalentin

After chatting with @JukkaL about this. I'll add back a flag to optionally switch the deprecated errors into notes.

@svalentin

@pre-commit-ci

@github-actions GitHub Actions

Diff from mypy_primer, showing the effect of this PR on open source code:

parso (https://github.com/davidhalter/parso)

SinbadCogs (https://github.com/mikeshardmind/SinbadCogs)

altair (https://github.com/vega/altair)

pydantic (https://github.com/pydantic/pydantic)

prefect (https://github.com/PrefectHQ/prefect)

optuna (https://github.com/optuna/optuna)

Tanjun (https://github.com/FasterSpeeding/Tanjun)

python-sop (https://gitlab.com/dkg/python-sop)

dd-trace-py (https://github.com/DataDog/dd-trace-py)

pywin32 (https://github.com/mhammond/pywin32)

spark (https://github.com/apache/spark)

alerta (https://github.com/alerta/alerta)

paasta (https://github.com/yelp/paasta)

comtypes (https://github.com/enthought/comtypes)

speedrun.com_global_scoreboard_webapp (https://github.com/Avasam/speedrun.com_global_scoreboard_webapp)

openlibrary (https://github.com/internetarchive/openlibrary)

dragonchain (https://github.com/dragonchain/dragonchain)

freqtrade (https://github.com/freqtrade/freqtrade)

materialize (https://github.com/MaterializeInc/materialize)

tornado (https://github.com/tornadoweb/tornado)

sockeye (https://github.com/awslabs/sockeye)

... (truncated 34 lines) ...```

ilevkivskyi

@ilevkivskyi

Would it be a good idea to add a config option to allow disabling deprecation warnings for warnings about specific fully qualified names?

Yes, I think it is a good idea. Maybe @tyralla will be interested in this. Btw another idea that appeared in an offline discussion is to actually show fully qualified names in deprecation errors/notes.

JukkaL

@tyralla

Yes, I think it is a good idea. Maybe @tyralla will be interested in this. Btw another idea that appeared in an offline discussion is to actually show fully qualified names in deprecation errors/notes.

Definitely good ideas. I could work on it in a few days.

tyralla added a commit to tyralla/mypy that referenced this pull request

Dec 6, 2024

@tyralla

x612skm pushed a commit to x612skm/mypy-dev that referenced this pull request

Feb 24, 2025

@cdce8p @x612skm