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

Small improvements to add additional context/explanations #461

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ejseqera
Copy link
Contributor

Optional enhancement: Added additional context to the body and notes to provide clearer explanations around concepts such as tuples and the handling of accessory files.

Copy link

netlify bot commented Nov 17, 2024

Deploy Preview for nextflow-training ready!

Name Link
🔨 Latest commit 454c7b0
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-training/deploys/673a740de7d4420008f0195a
😎 Deploy Preview https://deploy-preview-461--nextflow-training.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 site configuration.

Copy link
Member

@mribeirodantas mribeirodantas left a comment

Choose a reason for hiding this comment

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

I agree it's an improvement, but the ending worries me a bit. They will be converted into channels implicitly so I think there should be a better way to express the last mention of channels in the new version. What do you think?

@pinin4fjords
Copy link
Collaborator

pinin4fjords commented Nov 18, 2024

Yeah, I'm with @mribeirodantas on this one. I actually don't like the bare file() syntax, because of the implicit channel creation I don't think there's a principled reason to use it (besides maybe if you want to include the same file in multiple channels while initialising it once). So I disagree even with the original text - it's just a laziness/ convenience thing.

I'd rather we encouraged explicit channel creation, because I think it's easier to read.

@ejseqera
Copy link
Contributor Author

I see your points. I believe that there’s room to provide more context for newer developers about when and why to use the file() syntax in scenarios like this. How about reframing it like this:

"While main data inputs are streamed dynamically through channels, there are two approaches for handling accessory files. The recommended approach is to create explicit channels, which makes data flow clearer and more consistent. Alternatively, the file() function to create variables can be used for simpler cases, particularly when you need to reference the same file in multiple processes - though be aware this still creates channels implicitly. For example: ..."

This way, we emphasize explicit channel creation as the best practice while still introducing file() as an alternative for convenience. Do you think this balances the guidance appropriately?

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.

3 participants