Skip to content

Use Priority to override existing executor #6135

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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

Lehmann-Fabian
Copy link
Contributor

Since ServiceName.important is deprecated, I added support for the Priority annotation to work with the same behavior as other Nextflow Extension Points.

Copy link

netlify bot commented May 28, 2025

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit bccc6f6
🔍 Latest deploy log https://app.netlify.com/projects/nextflow-docs-staging/deploys/6846d74fb3ea840008e84c04
😎 Deploy Preview https://deploy-preview-6135--nextflow-docs-staging.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify project configuration.

@bentsherman bentsherman changed the title Use Priority to (not) overwrite Executor Use Priority to override existing executor May 30, 2025
@bentsherman
Copy link
Member

Plugins.getPriorityExtensions() is normally used to load plugins in a priority-aware manner, and should ideally be used here as well. Though I'm not sure why the plugin manager is provided here instead of just using Plugins

@Lehmann-Fabian
Copy link
Contributor Author

Lehmann-Fabian commented May 30, 2025

I think we can merge both constructors now as the manager isn't used anymore. What do you think?

@pditommaso pditommaso force-pushed the master branch 3 times, most recently from b4b321e to 069653d Compare June 4, 2025 18:54
@pditommaso
Copy link
Member

The ServiceName.important is deprecated, but there's no plan to remove it. Therefore I don't see a parallel logic necessary

@Lehmann-Fabian
Copy link
Contributor Author

Thank you for the clarification. However, I generally assumed that you eventually will remove deprecated elements. Therefore, I believe it's good practice to support both the deprecated and the new logic in parallel. This allows plugins to continue using important as long as it's available and ensures a smooth transition once it's removed, maintaining backward compatibility. This new approach is also consistent with how other extension points are currently managed.
If there is no plan to deprecate the important feature, should I remove the @Deprecated annotation?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants