Skip to content

Initial idlelib stubs #9193

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 and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 33 commits into from
Closed

Initial idlelib stubs #9193

wants to merge 33 commits into from

Conversation

CoolCat467
Copy link
Contributor

A lot of files are complete, but a few big ones like pyshell are missing a lot. I hope this will be a helpful start to adding complete support for idlelib.

@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

Hi! Are these stubs for the stdlib idlelib? If so, they're in the wrong directory — you need to put them in the stdlib/ directory rather than the stubs/ directory.

Also, can I ask what your use case is for these stubs? I think idlelib is only really meant to be used as an app — you're not really meant to import anything directly from idlelib. Due to PEP 434, idlelib is also exempted from CPython's usual backwards-compatibility guarantees, which might pose some interesting challenges for us in terms of maintaining these stubs.

@sobolevn
Copy link
Member

sobolevn commented Nov 14, 2022

I also think that the initial exception of idlelib was on purpose. People probably should not ever import something from idlelib, it is a text editor - not a library.

You did a massive (and great) work of annotating this package, but I think it was worth discussing first :(

@JelleZijlstra
Copy link
Member

It doesn't seem like we've discussed including idlelib in much detail; I only found this comment from @Akuli: #1019 (comment). I wouldn't necessarily be opposed to adding stubs for idlelib if it's useful for users, but it's definitely a serious maintenance risk.

@CoolCat467
Copy link
Contributor Author

I've found it useful for writing IDLE extensions, which was really the main reason I started working on them. Sorry I hadn't discussed it first, I was reading mypy's documentation about stub files to start this.

@AlexWaygood What would be best to go about changing what directory things should go to? Remove from this one and transfer to stdlib/ or make a new pull request?

@AlexWaygood
Copy link
Member

@AlexWaygood What would be best to go about changing what directory things should go to? Remove from this one and transfer to stdlib/ or make a new pull request?

Yeah, just move them from one directory to another and commit it -- no need to make a new pull request :) You don't need to worry about git history in a typeshed PR; it'll all be squash-merged in the end anyway.

@CoolCat467 CoolCat467 reopened this Nov 15, 2022
@github-actions

This comment has been minimized.

@AlexWaygood
Copy link
Member

I fixed a few easy-to-fix blocking things (adding it to VERSIONS, etc.), but before we go any further we need to decide if it's a good idea to accept these stubs.

Personally: I'd vote "no". The PEP 434 exemption from backwards compatibility scares me. I don't like the idea of stubtest suddenly failing in CI for every patch release of CPython because of idlelib feature backports. But I'd like to hear the opinions of other maintainers :)

@github-actions

This comment has been minimized.

@JelleZijlstra
Copy link
Member

Pinging @terryjreedy as the maintainer of IDLE (not asking for any work from you in terms of reviewing these stubs, but curious to hear your thoughts).

As I see it the main arguments in favor of including idlelib stubs in typeshed are:

  • typeshed's job is to provide type annotations for the stdlib, and idlelib is part of the stdlib.
  • Some users are using idlelib directly in their code, so having types for it would be useful

And the main arguments against are:

  • idlelib doesn't follow CPython's normal deprecation policy, so new features are freely backported. This makes it harder to keep the stubs up to date.
  • idlelib is primarily designed as an application, not a library. It's not meant to be used outside of IDLE.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@terryjreedy
Copy link
Member

I have never used static typing and have not previous seen non-toy stub files and have little idea of the different types of stub file tests. But I have followed some of the discussion of typing syntax in .py files.

Some types of 'foreign' code using idlelib:
0. Code that calls idlelib code as part of its function. (Example: turtledemo uses colorizer.)

  1. Code that calls the documented startup functions. (Example: Windows and Mac IDLE icons.)
  2. Extensions that use the narrow documented interface. (Example: idlelib.zzdummy, also for testing.)
  3. Extensions that patch idlelib more extensively to alter IDLE behavior.
    1 and 2 are supported, but do not really need this. 0 and 3 might use this, but are not supported (although we might try to not break things we know are used).

I could imagine using this myself for refactoring or other idlelib code improvements. But it is not complete, clear, nor correct.

  • Incomplete 1: imports not needed for typing are omitted. This initially made me think that there stubs were based on an old idlelib.
  • Incomplete 2: some things needed are also omitted. The first error report for Ubuntu 3.11 stems from this AutoComplete class method.
    @classmethod
    def reload(cls):
        cls.popupwait = idleConf.GetOption(
            "extensions", "AutoComplete", "popupwait", type="int", default=0)

popupwait, an int, is not in the stub, but it is used in a call within another method. (A bug is the stubmaker?)

  • Unclear 1: adding redundant 'as x' to nearly all idlelib imports is distracting. And why this difference in autocomplete.pyi?
from idlelib.editor import EditorWindow
from idlelib.hyperparser import HyperParser as HyperParser
  • Unclear 2: non-method class vars are listed above the class, where globals are (see editor.py versus editor.pyi). Assignments to self within methods are listed above the method, but since they are not mixed with anything else, I somewhat got used to this.

  • Incorrect: the event parameters of event handlers are (generally) typed Event[Any]|None. If the event is ignored, but is always passed as an Event, and the handler could be called with anything, but currently never is called directly by idlelib (doubleclick_event in autocomplete_w), the current type is either too broad or too narrow. Should it be Event[Any] or object? If the event is used without a guard (keypress_event, same file), None is wrong. But there are cases where the handler is called with both an event and without, and is either ignored or has guarded access.

In looking at corresponding code, I noticed that some None defaults (x=None) are tested with if (not) x rather than if x is (not) None. In some cases (especially strings), such as a filename passed to open(), the default could be "" (an invalid filename).

I am not sure that the potential benefit is worth the effort needed to squash the numerous remaining errors.

@AlexWaygood
Copy link
Member

AlexWaygood commented Nov 21, 2022

Thanks @terryjreedy for chiming in, that's helpful! I'll just respond to two of the points that you made above:

Incomplete 1: imports not needed for typing are omitted. This initially made me think that there stubs were based on an old idlelib.

Yes, this is intentional. Stub files are useful for many purposes -- they can often serve as documentation for a human reader. But the fundamental purpose of typeshed's stubs is a machine-readable format for a type checker. Imports that are not required for typing purposes are generally removed from the stub; if there are imports that are unnecessary for that purpose, our CI will fail.

Unclear 1: adding redundant 'as x' to nearly all idlelib imports is distracting. And why this difference in autocomplete.pyi?

Personally, I'm also not a massive fan of this idiom. However, PEP 484 states that imports in stub files should generally be considered "private to the stub" -- i.e., if a stub file foo.pyi has from re import Pattern at the top of the file, a type checker should emit an error if a user tried to do from foo import Pattern. One of the exceptions to this rule, however, is if you use the "redundant" from eggs import spam as spam idiom in foo.pyi, which is a way of declaring to the type checker that a specific import is meant to be re-exported from that file, and that it would therefore be okay for a user to do from foo import spam, even though spam did not originate in foo.pyi.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

@github-actions
Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@AlexWaygood
Copy link
Member

Thanks for the PR, @CoolCat467, but I'm going to reject this, I'm afraid. I still think adding stubs for idlelib would be a bad idea, and it doesn't seem like any other maintainers are strongly in favour of adding these stubs. CPython's official position is that the entire directory is an implementation detail that can be changed at any time, even in backports: see PEP 434, and also recent comments such as python/cpython#101154 (comment).

If you think these stubs will be useful for other people, you'll still be able to add stubs to PyPI as an idlelib-stubs package! They just won't be part of typeshed, I'm afraid :)

Good luck!

@CoolCat467
Copy link
Contributor Author

Ok, thank you for the information.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants