`nextflow format hello-nextflow/` #565

ewels · 2025-03-13T20:01:30Z

Ran the new upcoming nextflow format command on the hello-nextflow directory.
Once this is out, we'll start to run this formatter on a lot of nf-core code and it should quickly become the established formatting style for Nextflow code. I figure it's good to get ahead of the game and get the training to follow this style too.

No manual changes here at all, purely the automated formatting changes. We'll need to update the markdown code to reflect these changes.

Note that in other docs I've used mkdocs material "snippets" to directly include code files into the markdown without copying + pasting, which makes these kinds of changes easier, not sure if it's worth considering here as we update or not worth the effort.. See docs, docs and example:

training/docs/basic_training/containers.md

Lines 447 to 449 in ce85956

    
           ```yaml title="nf-training/env.yml" linenums="1" 
        
           --8<-- "nf-training/env.yml" 
        
           ```

Once we have sign off on the use of this formatting tool and the code style it produces, I can update this PR to also adjust the markdown.

Once that's done we should apply it to all other material, then set up pre-commit to automatically check (/ apply on comment) this auto-formatting.

Phil

netlify · 2025-03-13T20:02:33Z

✅ Deploy Preview for nextflow-training ready!

Name	Link
🔨 Latest commit	`d9d5ab7`
🔍 Latest deploy log	https://app.netlify.com/sites/nextflow-training/deploys/67d3399d0d36a2000713178c
😎 Deploy Preview	https://deploy-preview-565--nextflow-training.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

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

bentsherman · 2025-03-13T20:24:56Z

Please let me know if you have opinions about formatting. I will hear you out. Get your views in before things become set in stone!

vdauwera

Added some questions/comments that may be more for @bentsherman

vdauwera · 2025-03-17T20:12:06Z

hello-nextflow/hello-channels.nf


    script:
    """
-    echo '$greeting' > output.txt
+    echo '${greeting}' > output.txt


are the curly braces going to be mandatory? why do we need them?

They are optional in certain cases like simple variable names. The formatter doesn't try to be smart here, it just always uses braces

As for whether we should put them everywhere, there is a trade-off between consistency and shortcuts

Using curly braces everywhere requires a bit more typing but makes the code more consistent, therefore easier to read

Omitting the curly braces where possible saves a bit on typing but makes the code less consistent, thus the reader has to work a bit more to understand the different syntax forms

So I think it comes down to how much time we spend reading vs writing code. At least in software engineering, we spend way more time reading code, so I tend to favor consistency

I think it's easier to teach consistency as well. So I like this change.

vdauwera · 2025-03-17T20:13:33Z

hello-nextflow/hello-config.nf

@@ -1,4 +1,10 @@
 #!/usr/bin/env nextflow
+// Include modules


Is the idea here that module includes should go before anything else? Is there a functional reason or is it purely stylistic? Any interactions wrt setting any variable values?

In principle, it's just a convention as it shouldn't matter where you put the includes

In practice, users sometimes put the params first so that they are propagated to the included modules as a side effect. We are trying to move away from that pattern anyway

vdauwera · 2025-03-17T20:15:17Z

hello-nextflow/hello-config.nf

-    greeting_ch = Channel.fromPath(params.greeting)
-                        .splitCsv()
-                        .map { line -> line[0] }
+    greeting_ch = Channel


I'm not sure I like having to put the .fromPath() bit on the next line. I would keep that like it was before tbh. That way you have the first line do a full thing, then the next lines modify that thing. If you have just greeting_ch = Channel on one line that feels... interrupted

Honestly I agree. I would just need to add a special case in the formatter. Though I wonder if we would feel the same about any old foo.bar() or if it's just with channel factories?

For me I would do this for any old foo.bar() as well I think.. The first line should contain the first action. Whether it's Channel or foo, neither of these are actions, only the starting points.

When I imagine something like this:

ch_samples .filter { sample -> sample.n_reads >= 1_000_000 } .map { sample -> sample.id }

I think I prefer both calls to be on their own line. I think I treat channel factories differently in my head because the Channel and the factory call are sort of a unit, but in general it seems like the "starting point" should be separate from any transformations performed on it

the format command will now put Channel.fromPath() on a single line, as well as any other channel factory

vdauwera · 2025-03-17T20:15:56Z

hello-nextflow/hello-config.nf

@@ -30,7 +31,7 @@ workflow {
    collectGreetings(convertToUpper.out.collect(), params.batch)

    // emit a message about the size of the batch
-    collectGreetings.out.count.view { "There were $it greetings in this batch" }
+    collectGreetings.out.count.view { "There were ${it} greetings in this batch" }


are the extra curlies necessary here?

also should we be explicitly naming the variable like we do later with line -> line[0] etc?

See my point above about curly braces. As for the implicit it, yes I would recommend as a best practice that you declare an explicit name such as line. Right now it's only a "paranoid" warning but it will eventually become an error

I thought I got all of the $its in #538 - must have missed this one.

vdauwera · 2025-03-17T20:25:38Z

@ewels Before this can go in we're also going to have to update a bunch of line numbers, check the explanatory text for any code that is quoted and needs to be updated to match etc., so this will still need some substantial effort.

Also, do you have any thoughts on how this will make the code different in a lot of places compared to what you show in the videos? I know we can tell people to expect minor changes but a few of these aren't really minor, at least for newcomers (eg chunks of code that move to a different part of the file) and in general it's going to add 'noise' in terms of people trying to follow what's going on on screen and on their own machine.

So I'm a little reluctant to push this out right away if there's not a driving need for it. At some level I'd rather sit on it until we feel we may need to update the recordings themselves (hopefully not for some number of months as they are so good and it takes so much effort to make), OR there's a strong reason for why we need the formatting to be strictly aligned.

ewels · 2025-03-17T20:40:05Z

Yup, totally agree - the material will need some significant changes. Also reordering some sections, eg. introducing workflows first then processes.

I wanted to get feedback on the formatter's style choices first really, to see how many changes it would make and see if we agree with them. Then we can adjust it before it goes live in Nextflow.

Agree that we shouldn't rush this, and happy to leave it for quite a while before doing.

That said, maybe we can progressively implement it, using it for any new courses / material from now on. And maybe applying to the courses that don't have videos. Does that sound ok?

adamrtalbot · 2025-03-18T16:30:57Z

hello-nextflow/solutions/1-hello-world/hello-world-3.nf

+workflow {
+
+    // emit a greeting
+    sayHello()
+}
+


I dislike having workflow above processes. I use this order:

include

params

functions

processes

workflows

anonymous workflow

I'm debating whether to add an option to control the order (e.g. "Formatting > Entry workflow position")

Or maybe just not try to re-order the definitions at all?

I've always done processes first then workflows, but I'm coming around to it.

I don't mind making @adamrtalbot feel uncomfortable, auto-formatters have that effect on everyone 😅 Though maybe this goes beyond what most formatters do?

I'll survive if it gets reordered, but do we have a particular order that is preferred? It feels weird to be opinionated about something that isn't consequential (not that it's stopped me before).

Isn't that what auto-formatters do? 😆 Basically no formatting is consequential. It's mostly for convention and consistency, so that folks get used to navigating pipeline code in the same way everywhere.

Any colour, as long as it's black.

Most languages don't have this problem because they just have functions, no processes / workflows.

I do think that most people put their workflows at the bottom. This is also the convention that Paolo used when he introduced DSL2 in the docs, which could explain it.

I'm starting to think the formatter should just preserve the user's order for now

Agreed - or at least make it opt in, off by default for now.

..Maybe with includes as an exception? I do like having them at the top and alphabetically sorted 👀

Yeah, feature flags / includes / params should go first no matter what

the format command now will not try to re-order your declarations unless you enable it, should make the formatting less intrusive overall

nextflow format .

d9d5ab7

vdauwera reviewed Mar 17, 2025

View reviewed changes

vdauwera added improve New feature or request hello series labels Mar 17, 2025

adamrtalbot reviewed Mar 18, 2025

View reviewed changes

ewels marked this pull request as draft March 19, 2025 23:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`nextflow format hello-nextflow/` #565

`nextflow format hello-nextflow/` #565

ewels commented Mar 13, 2025

netlify bot commented Mar 13, 2025 •

edited

Loading

bentsherman commented Mar 13, 2025

vdauwera left a comment

vdauwera Mar 17, 2025

bentsherman Mar 17, 2025

bentsherman Mar 17, 2025

kenibrewer Mar 19, 2025

vdauwera Mar 17, 2025

bentsherman Mar 17, 2025

vdauwera Mar 17, 2025

bentsherman Mar 17, 2025

ewels Mar 19, 2025

bentsherman Mar 19, 2025

bentsherman Apr 10, 2025

vdauwera Mar 17, 2025

vdauwera Mar 17, 2025

bentsherman Mar 17, 2025

ewels Mar 19, 2025

vdauwera commented Mar 17, 2025

ewels commented Mar 17, 2025

adamrtalbot Mar 18, 2025

bentsherman Mar 18, 2025

ewels Mar 19, 2025

adamrtalbot Mar 19, 2025

ewels Mar 19, 2025

bentsherman Mar 19, 2025

ewels Mar 20, 2025

ewels Mar 20, 2025

bentsherman Mar 20, 2025

bentsherman Apr 10, 2025

	```yaml title="nf-training/env.yml" linenums="1"
	--8<-- "nf-training/env.yml"
	```

nextflow format hello-nextflow/ #565

Are you sure you want to change the base?

nextflow format hello-nextflow/ #565

Conversation

ewels commented Mar 13, 2025

netlify bot commented Mar 13, 2025 • edited Loading

✅ Deploy Preview for nextflow-training ready!

bentsherman commented Mar 13, 2025

vdauwera left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vdauwera commented Mar 17, 2025

ewels commented Mar 17, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`nextflow format hello-nextflow/` #565

`nextflow format hello-nextflow/` #565

netlify bot commented Mar 13, 2025 •

edited

Loading