Skip to content

Conversation

KRRT7
Copy link
Contributor

@KRRT7 KRRT7 commented Jul 1, 2025

Fixes (partly) #2547

PR Checklist:

  • [ x] All new features have been tested
  • [ x] All new features have been documented
  • [x ] I have read the CONTRIBUTING.md file
  • [x ] I will abide by the code of conduct

@KRRT7 KRRT7 changed the title Fix type hints implement Lazy Loading for Cocoa core imports Jul 1, 2025
@KRRT7 KRRT7 marked this pull request as draft July 1, 2025 07:13
@KRRT7 KRRT7 force-pushed the fix-type-hints branch from f578ad7 to 4d75502 Compare July 1, 2025 07:32
@mhsmith
Copy link
Member

mhsmith commented Jul 2, 2025

I'm not sure if you're aware of how to run the testbed on your own machine, but there are instructions here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My two cents is that the tests_backend stuff aren't being run as tests -- they just support the test files in the tests directory in the testbed, so this file isn't been ran at all. Not sure if this is right, but I think you'd need to generalize this to not write toga_cocoa but to work out a name to change the tested import depending on backend in the testbed tests directory, and then skip the test on everything besides Cocoa for now.

Copy link
Member

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

My immediate impression is that I don't think this PR is doing what you think it is doing.

The giveaway is that the toga_cocoa/__init__.py module is importing everything from factory. At present, import toga_cocoa doesn't import all the parts of the factory module into the toga_cocoa namespace. So - this PR adds a bunch of lazy symbols to the toga_cocoa namespace... where they will never be used.

If we're going to use lazy loading on backends, it needs to be on the factory module itself. That's the API that toga-core uses to load platform implementations (see all the impl = factory.WidgetName() calls in toga-core.

Also - as @johnzhou721 has noted, the test_import that you've added won't ever be executed. That said - if the lazy import is done right, it won't need any additional tests - because the existing codebase is testing every valid import. All this change would do is modify how that import is implemented, making it (hopefully) faster.

@johnzhou721
Copy link
Contributor

Also - as @johnzhou721 has noted, the test_import that you've added won't ever be executed. That said - if the lazy import is done right, it won't need any additional tests - because the existing codebase is testing every valid import. All this change would do is modify how that import is implemented, making it (hopefully) faster.

Agreed. But I think we should have an explicict test about failed imports. Cocoa has now achieved 100% functionality coverage, so there's no Cocoa backend doesn't implement messages that come up, and we still need to verify that this functionality w/ doesn't impl messsages work correctly.

@freakboy3742
Copy link
Member

Agreed. But I think we should have an explicict test about failed imports. Cocoa has now achieved 100% functionality coverage, so there's no Cocoa backend doesn't implement messages that come up, and we still need to verify that this functionality w/ doesn't impl messsages work correctly.

Sure - but that's more of a "filling a testing gap that is currently hidden", rather than anything directly related to this PR. I've added #3632 to track this as an idea.

@johnzhou721
Copy link
Contributor

Wow... didn't realize it wasn't covered now!!! My eyes tend to ignore gray things (comments in Xcode as I don't even have a proper IDE just random apps that happens to open Python files), I guess.

@mhsmith
Copy link
Member

mhsmith commented Jul 10, 2025

if the lazy import is done right, it won't need any additional tests - because the existing codebase is testing every valid import. All this change would do is modify how that import is implemented, making it (hopefully) faster.

As we discussed in the toga-core lazy import PR, we should still have a test if possible, otherwise it would be easy to reintroduce an eager import by accident and not notice for years. The same approach can be used in this PR.

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.

4 participants