poetry-core icon indicating copy to clipboard operation
poetry-core copied to clipboard

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

Open dylan-robins opened this issue 1 year ago • 6 comments

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 avatar Mar 28 '24 16:03 dylan-robins

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 :)

dylan-robins avatar Mar 28 '24 18:03 dylan-robins

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.

radoering avatar Mar 30 '24 17:03 radoering

In https://github.com/python-poetry/poetry/pull/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.

dimbleby avatar Mar 30 '24 18:03 dimbleby

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:

https://github.com/python-poetry/poetry-core/blob/c47aad19d42793ccc7bb466790936230f115c635/src/poetry/core/factory.py#L215-L225

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?

dylan-robins avatar Mar 31 '24 11:03 dylan-robins

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

sonarqubecloud[bot] avatar Mar 31 '24 11:03 sonarqubecloud[bot]

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

dimbleby avatar Mar 31 '24 22:03 dimbleby