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

refactor: Migrate to the flat layout #488

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

kshramt
Copy link
Contributor

@kshramt kshramt commented Mar 20, 2024

If this PR is accepted, I intend to follow up with another PR to add py.typed to address #305 .

@kshramt kshramt changed the title Migrate to the flat layout chore: Migrate to the flat layout Mar 21, 2024
@kshramt
Copy link
Contributor Author

kshramt commented Mar 22, 2024

(I am not sure why "5 workflows awaiting approval" is not shown.)

@kshramt kshramt changed the title chore: Migrate to the flat layout refactor: Migrate to the flat layout Mar 25, 2024
@kshramt
Copy link
Contributor Author

kshramt commented Mar 27, 2024

I plan to rebase the PR after the review.

@alexanderankin
Copy link
Member

marking as draft because i think we need a larger discussion before making this kind of change (for example, i am not convinced but i havent thought about it too much, maybe i am missing an argument for (or against))

@alexanderankin alexanderankin marked this pull request as draft March 30, 2024 23:26
@santi
Copy link
Collaborator

santi commented Apr 4, 2024

@kshramt I read through the referenced issue, but I don't get the context of why we would want a flat package hierarchy? Having all community packages in the module directory is quite clean, but if there are any strong arguments on why it shouldn't be that way I am eager to listen.

@kshramt
Copy link
Contributor Author

kshramt commented Apr 6, 2024

@santi, thanks for the comment. I appreciate the neatness of the current structure but have observed some challenges, especially for those new to the unique packaging patterns. When I set out to integrate MyPy support, my initial expectation was straightforward: simply run touch py.typed and address type errors. However, figuring out how to properly include py.typed in the existing structure—and how to run MyPy and similar tools—wasn't as intuitive as I had hoped. This deviation from more conventional Python project structures required additional research and effort, which could be a hurdle for new contributors.

Additionally, specific adjustments, such as those detailed in 507e466#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R33 and 507e466#diff-50c86b7ed8ac2cf95bd48334961bf0530cdc77b5a56f852c5c61b89d735fd711R234, are rendered unnecessary with a transition to the flat layout. Migrating to this layout not only makes the project structure more intuitive for everyone involved but also minimizes maintenance overhead associated with tools' structural requirements.

I believe that adopting the more common flat layout could make the project more accessible and easier to maintain. I'm genuinely interested in your perspective on this issue. Do you have any specific concerns with the flat layout, or are there other solutions you believe might be more effective?

@alexanderankin
Copy link
Member

the part of the project under core/ is the only part which truly needs to be maintainer friendly. the rest is a demonstration of what core/ can do. e.g., the segmentation is what allows a partial effort to type the package, instead of needing to do the whole thing at once.

@kshramt
Copy link
Contributor Author

kshramt commented Apr 7, 2024

the segmentation is what allows a partial effort to type the package, instead of needing to do the whole thing at once.

I understand the rationale behind segmentation for facilitating partial typing efforts. However, it's worth noting that the flexibility for partial typing can still be maintained within the flat layout. As outlined in https://peps.python.org/pep-0561/#packaging-type-information , package maintainers aiming to support type checking must include a py.typed marker file in their packages to indicate support for typing. Importantly, this marker file's presence applies recursively, meaning if a top-level package is marked with py.typed, all its sub-packages are also considered to support type checking.

If I interpret correctly, this implies that by placing a py.typed file within testcontainers/core, and then running mypy testcontainers/core, we effectively achieve partial typing. This approach aligns with PEP 561's guidelines and ensures that we can selectively type parts of the project without necessitating a complete overhaul at once. This strategy should allow us to enjoy the benefits of the flat layout while still accommodating partial typing efforts as needed.

@kshramt
Copy link
Contributor Author

kshramt commented Apr 7, 2024

For distribution, according to python/typing#1429, a partially annotated package with the root py.typed file also appears to be acceptable.

@jankatins
Copy link
Contributor

Personally, I would argue that everything should be typed and failing to do so is a bug. At least in my books, a untyped library reflects badly on how I would judge the library nowadays (looking at confluent kafka...).

@CarliJoy
Copy link
Contributor

I am with @kshramt on this. But I would prefer a src layout instead of flat layout.

Doing so it more clear what is actually packages and prevents some problems testing the wrong package....

@totallyzen
Copy link
Collaborator

Hey all!

Since I wrote the refactor to the current core and modules, I can chime in a bit.

There is a very simple reason why it is core and modules -> it is because all major testcontainers language flavors use these. I don't think I'd want to deviate from that as it allows the TC mainline documentation to converge and easily match support across languages (automation can be built for it, and issues tracked across language flavours)

I wish we had better automation and support from the TC staff (I'm only a volunteer myself like @alexanderankin) but this is what we've got for now.

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.

6 participants