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

[Feature Request]: Autodiscovery as its own package #854

Closed
Wulfheart opened this issue Dec 13, 2024 · 3 comments · Fixed by #861
Closed

[Feature Request]: Autodiscovery as its own package #854

Wulfheart opened this issue Dec 13, 2024 · 3 comments · Fixed by #861
Labels
Core Uncharted Waters This feature request has been considered and approved!

Comments

@Wulfheart
Copy link
Contributor

Description

Some days ago I saw a thread about it one the project's discord but unfortunately I am unable to find it again.
I really like the Autodiscovery feature and I would like to use this specific component in non-tempest projects. Therefore, I am proposing the following:

  1. Move Tempest\Core\{DiscoversPath,Discovery,DiscoveryException,DiscoveryItems,DicoveryLocation,IsDiscovery,DoNotDiscover} to Tempest\Discovery
  2. Tempest\Core\{DiscoveryCache,DiscoveryCachingStrategyChangedException,et al.} stay in core as they are tied to the Tempest specific implementation
  3. Tempest\Core\Kernel\LoadDiscoveryClasses will also remain where it is right now as users of the Discovery package will have to provide their own strategy on how to load.
  4. The package tempest/discovery will have a dependency on tempest/reflection. As far as I can see this should be enough.

Benefits

I really enjoy the autodiscovery component and would love to use it in projects outside of tempest. I can also provide a PR if you are not opposed to it.

@Wulfheart Wulfheart added Feature Request Request a feature that is not currently in the framework. Triage New issues that need to be reviewed by the Tempest team. labels Dec 13, 2024
@aidan-casey
Copy link
Member

@Wulfheart I haven't moved around any namespaces yet, but what are your thoughts on the implementation in #815?

@innocenzi innocenzi added Core Uncharted Waters This feature request has been considered and approved! and removed Triage New issues that need to be reviewed by the Tempest team. labels Dec 13, 2024
@brendt
Copy link
Member

brendt commented Dec 14, 2024

@aidan-casey I don't really see how the two PRs relate. Yours is about performance. Agreed, you also made some structural changes, but @Wulfheart 's proposal seems simpler and also makes sense.

Here are my thoughts:

  • @Wulfheart I'm open for a PR that pulls discovery into a separate package
  • From what I understand, the standalone package would have no link to the container? I wonder how that would play out in practice. We'd probably need bridges for popular frameworks.
  • @aidan-casey after this refactor, we explore your changes, because the performance part does matter. But let's keep each PR focused on one thing.

@brendt
Copy link
Member

brendt commented Jan 9, 2025

As a followup:

Tempest\Core\Kernel\LoadDiscoveryClasses will also remain where it is right now as users of the Discovery package will have to provide their own strategy on how to load.

I wonder if it makes sense to have a "default strategy" included in tempest/discovery? @Wulfheart feel free to share input based on your experiences

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Core Uncharted Waters This feature request has been considered and approved!
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants