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

Migrate anonymous Mapping[str, Any] to TypedDicts where their content is fully defined. #716

Open
wants to merge 10 commits into
base: main
Choose a base branch
from

Conversation

dylan-robins
Copy link

When developing a plugin for Poetry, I found that despite the "include", "exclude" and "package" mapping content being fully defined in the documentation, no type hints were available to help me. Therefore I propose this minor change. It has no impact on current functionality, however with this IDEs like PyCharm and VSCode are aware of the structure of these dicts.

  • Added tests for changed code. NOT APPLICABLE
  • Updated documentation for changed code. NOT APPLICABLE (I think)

Other similar changes may be useful elsewhere in the code. I'll continue to explore the codebase and see what I can do.

@dylan-robins
Copy link
Author

I do not know why the integration tests are failing, I've added typing_extensions as a dependency to the main pyproject.toml however the integration tests still indicate the following:

  File "/home/robinsdy/poetry-core/src/poetry/core/masonry/api.py", line 12, in <module>
    from poetry.core.factory import Factory
  File "/home/robinsdy/poetry-core/src/poetry/core/factory.py", line 15, in <module>
    from poetry.core.packages.project_package import BuildConfigSpec
  File "/home/robinsdy/poetry-core/src/poetry/core/packages/project_package.py", line 11, in <module>
    from typing_extensions import NotRequired
ModuleNotFoundError: No module named 'typing_extensions'

I am not entirely sure what this is, I'll look further into how your tests are constructed tomorrow but if anyone else wants to sort this out that'd be great :)

@radoering
Copy link
Member

In general, our stance is that poetry-core (as a build backend) should not have any dependencies. Since typing-extensions is a small package we might think about vendoring it. However, I'm not sure if it's worth it and would like to hear more opinions.

@dimbleby Since you are familiar with vendoring and contributing many typing improvements, I'd appreciate your opinion.

@dimbleby
Copy link
Contributor

dimbleby commented Mar 30, 2024

In python-poetry/poetry#9251 we have established that so long as all imports of typing-extensions can be moved behind if TYPE_CHECKING:, it is not actually necessary to have typing-extensions installed in the environment - and therefore not necessary to depend on it.

Over in poetry I have objected to this approach: it feels fragile and non-obvious and who cares about another dependency anyway?

Over here it still feels fragile and non-obvious - but the case for wanting to avoid the dependency is stronger.

So that is an option.

@dylan-robins
Copy link
Author

dylan-robins commented Mar 31, 2024

Ah I see, thanks for the info @dimbleby. I also don't like this approach, but I do see the reasoning behind it.

Another side-effect of this that I don't like is that I've had to add if TYPE_CHECKING-guarded casts inside poetry.core.factory.Factory.configure_package:

for include in config["include"]:
if not isinstance(include, dict):
include = {"path": include}
formats = include.get("format", [])
if formats and not isinstance(formats, list):
formats = [formats]
include["format"] = formats
if TYPE_CHECKING:
include = cast(IncludeSpec, include)
package.include.append(include)

I don't like this at all, like you said it seems non-obvious and very fragile. Any ideas for how to make mypy happy without having this ugliness in the code?

Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@dimbleby
Copy link
Contributor

Iirc you can cast to the string "Foo" rather than the type Foo

Comment on lines 202 to +204
build = config["build"]
if not build:
build = {}
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
build = config["build"]
if not build:
build = {}
build = config.get("build", {})

?

@@ -66,7 +66,7 @@ def _module(self) -> Module:
formats = [formats]

if (
formats
len(formats) > 0
Copy link
Member

Choose a reason for hiding this comment

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

Does that make a difference? Isn't the former more Pythonic?

Comment on lines +25 to +37
BuildConfigSpec = TypedDict(
"BuildConfigSpec",
{"script": NotRequired[str], "generate-setup-file": NotRequired[bool]},
)

class PackageSpec(TypedDict):
include: str
to: str
format: list[SupportedPackageFormats]

class IncludeSpec(TypedDict):
path: str
format: list[SupportedPackageFormats]
Copy link
Member

Choose a reason for hiding this comment

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

Since these were Mappings before and thus immutable for a reason (see comment in ProjectPackage.__init__), we probably should declare all attributes as ReadOnly.

class PackageSpec(TypedDict):
include: str
to: str
format: list[SupportedPackageFormats]
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be a Sequence.


class IncludeSpec(TypedDict):
path: str
format: list[SupportedPackageFormats]
Copy link
Member

Choose a reason for hiding this comment

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

Should probably be a Sequence.

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.

3 participants