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

Automatically treat nested modules as submodules #4308

Merged
merged 1 commit into from
Jul 9, 2024

Conversation

alex
Copy link
Contributor

@alex alex commented Jul 2, 2024

fixes #4286

@alex alex force-pushed the auto-submodule branch 2 times, most recently from a3005a5 to efebf51 Compare July 2, 2024 21:45
@alex
Copy link
Contributor Author

alex commented Jul 2, 2024

(This ended up being way simpler than I realized now that we support #[pyo3(submodule)]. If I realized it was so simple I'd have rolled it into the original :-))

Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

I think we probably have to treat this as a 0.23 thing as it is technically breaking to merge this into 0.22 now.

@@ -237,6 +237,7 @@ pub fn pymodule_module_impl(
)? {
set_module_attribute(&mut item_mod.attrs, &full_name);
}
item_mod.attrs.push(parse_quote!(#[pyo3(submodule)]));
Copy link
Member

Choose a reason for hiding this comment

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

I think this needs some additional tests.

If submodule is already present, will this error? Probably it will error for duplicate submodule setting already, the error message should ideally read "submodules of declarative modules don't need submodule attribute set".

We might want to allow for submodule = false or maybe #[pymodule(importable)] to allow turning this off. Maybe? (TBH I can't see a use case where users would want to nest declarative modules and then allow both to be importable as root modules too.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I cannot think of a valid use case for that scenario. If one does want it, it can be accomplished by declrating two top-level modules and exporting one into the other.

@davidhewitt davidhewitt added this to the 0.23 milestone Jul 5, 2024
@alex alex force-pushed the auto-submodule branch from efebf51 to 8e3c276 Compare July 5, 2024 12:54
@alex
Copy link
Contributor Author

alex commented Jul 5, 2024

Added a test case for the compilation error.

When you say backwards incompatible, are you referring to removing the PyInit for a nested module? As we both said, I can't think of a valid use case for that, but I suppose it is technically a behavioral change.

Either way, I can wait on this, since we can just manually mark things as submodules.

@alex alex force-pushed the auto-submodule branch from 8e3c276 to e5ab5ac Compare July 5, 2024 13:07
Copy link
Member

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Given that I've started merging changes for 0.23, this now LGTM, let's merge it. Thanks 👍

@alex alex added this pull request to the merge queue Jul 9, 2024
Merged via the queue into PyO3:main with commit e73112f Jul 9, 2024
40 of 41 checks passed
@alex alex deleted the auto-submodule branch July 9, 2024 13:25
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.

Declarative modules: submodule declarations are not considered covered
2 participants