Skip to content
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

Allow marking stubs as partial #94

Merged
merged 6 commits into from
Jun 1, 2023

Conversation

Avasam
Copy link
Contributor

@Avasam Avasam commented May 2, 2023

Closes #93

I'm sure this can be improved. Also please review and test carefully (I'm still running integration tests, but they're taking forever).

The basic idea is to infer a stub package being partial from finding ignore_missing_stub = true in the metadata. If we decide to add a specific "partial" entry in the metadata, the logic will stay the same, only the property's inner code will change.
Edit: went for a separate metadata key.

Alternatively typeshed could add the partial\n py.typed marker itself (assuming it can use packages instead of single files for single-file source w/o problem with stubtest)

@srittau
Copy link
Contributor

srittau commented May 3, 2023

In that case it would probably make sense to rename ignore_missing_stub to partial or partial_stub and clearly document what it does on the typeshed side. I'm a bit uncomfortable about adding the partial marker just by looking at some field named ignore_missing_stub.

@Avasam
Copy link
Contributor Author

Avasam commented May 3, 2023

In that case it would probably make sense to rename ignore_missing_stub to partial or partial_stub and clearly document what it does on the typeshed side. I'm a bit uncomfortable about adding the partial marker just by looking at some field named ignore_missing_stub.

I personally wouldn't rename [tool.stubtest].ignore_missing_stub (at least not due to this feature).

I infer from it, because having stubtest ignore missing stubs usually means that, well, there's missing stubs! (and so, the stub package should probably be marked as partial, so that we don't hide public API that is not yet typed).
I do recognize this may not always be the case in the future (as we both alluded to, ignore_missing_stub and partial are semantically different), so if you'd prefer going immediately for a different setting, let's do that 😃 (and the required changes in typeshed).

I see two ways of doing it (more suggestions welcome). Whichever solution should be well documented in typeshed.

  1. Let typeshed add py.typed markers itself. And a test to ensure it contains partial\n. stub_uploader then only needs to include them in its filters.
  2. Add a new metadata entry partial or partial_stub, keep this PR mostly as-is.

Now for example use cases where partial_stub = true and ignore_missing_stub = false:

  • pywin32: The maintainers stated not wanting to maintain separate stub files. But are open to inline typing (given they can be validated). So once ongoing obsolete code cleanup is done, the library would use inline types, and c-module extensions stubs would live in typeshed. I'd suppose pure-python modules would be excluded in stubtest. This is a very long-term maybe-plan.

partial_stub = false and ignore_missing_stub = true:

  • stubs that are skipped in stubtest (though skip + ignore_missing_stub wouldn't hurt)
  • stubs that would have thousands of lines in stubtest_allowlist files otherwise (I don't have a concrete example where this is a preferred solution over "fixing" whatever issue with a stub or tool that causes this, or using regex)
  • A bug in a type-checker that causes performance issue or incorrect behaviour when merging partial stubs.

@srittau
Copy link
Contributor

srittau commented May 5, 2023

I'd prefer to split those features, i.e. have a separate partial_stub key in the METADATA.toml file for the use cases you outlined and to avoid "hidden" side effects.

@Avasam Avasam changed the title Infer partial stubs and add marker Allow marking stubs as partial May 8, 2023
Copy link
Contributor

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Thanks! The mkdir/makedirs suggestions are optional, in case you like your solution better.

Co-authored-by: Sebastian Rittau <srittau@rittau.biz>
@Avasam Avasam requested a review from srittau May 20, 2023 03:11
Copy link
Contributor

@srittau srittau left a comment

Choose a reason for hiding this comment

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

Whoops, sorry for forgetting about this and thanks!

@srittau srittau merged commit 7dec7aa into typeshed-internal:main Jun 1, 2023
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.

Mark incomplete stubs as partial
2 participants