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

Document formal script syntax #5336

Merged
merged 29 commits into from
Oct 23, 2024
Merged

Document formal script syntax #5336

merged 29 commits into from
Oct 23, 2024

Conversation

bentsherman
Copy link
Member

Now that the docs are in better shape, we can begin work towards a formal description of the Nextflow language, as well as prepare the docs and tests for the strict syntax that will be introduced by #4613 and #4744 .

Refer to the changes to snippets and tests for examples of strict syntax:

  • replace implicit it closure parameter with explicit parameter
  • use quotes for process env input/output name
  • move top-level statements into process / function / workflow definitions

Currently the syntax description is split across several pages:

  • Scripts
  • Processes
  • Workflows
  • Modules

It would be nice to have a single page that describes the entire language. At the same time, I don't want to move these pages around much more, and they are too hefty individually to combine them.

Since these pages cover a lot of other topics like APIs and runtime behavior, maybe it would be better to add a new page e.g. "Reference > Syntax" which gives a top-down syntax description.

Looking to the Groovy docs as an example, I feel like the Scripts page could be expanded to also describe process/workflow/include syntax. It just feels weird to mention them here when there are already dedicated pages.

Copy link

netlify bot commented Sep 25, 2024

Deploy Preview for nextflow-docs-staging ready!

Name Link
🔨 Latest commit 3f6f377
🔍 Latest deploy log https://app.netlify.com/sites/nextflow-docs-staging/deploys/671928afdd4b3c000873be4e
😎 Deploy Preview https://deploy-preview-5336--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 site configuration.

@christopher-hakkaart
Copy link
Contributor

I agree having everything on the same page would be too much, and I'm also not sure if that would make the most sense for someone accessing the docs.

So I guess the question is where the bulk of the syntax should be described in detail (a single page or separate pages).

I wonder if there can be a compromise where the bulk or common syntax is described on a Syntax page but also have Syntax sections (e.g., Groovy closures). The "main" syntax page could have small sections about the other syntax sections and then link out, rather than build everything into one page.

@bentsherman
Copy link
Member Author

I was thinking something similar, have a top-level page that links out to the existing pages. Two questions around that:

  1. Do you think this page should be under "Developing pipelines" or a reference page?
  2. Do you think the page should absorb the current "Scripts" content or also link to it?

I think we could just repurpose the "Scripts" page with an outline like this:

  • (overview)
  • workflows (briefly describe and link out)
  • processes (briefly describe and link out)
  • functions
  • "basic syntax"
    • numbers, strings, lists, variables, closures, etc
    • link out to "Syntax" reference page

Then add a "Syntax" reference page which is actually a full description of the script grammar. This way the "Scripts" page doesn't have to be comprehensive on basic syntax, it can just provide the most important examples. The reference page can get very specific on things like punctuation, what kind of code is allowed where, etc

In any case, we need to have a broader conversation on how to evolve the docs from here, but I think for now I can do a first draft and let us iterate on that

@christopher-hakkaart
Copy link
Contributor

I was thinking something similar:

  • Scripts (living under developing pipelines) have a high-level overview with minimal examples and lots of links to reference pages. The purpose is to describe the language and how it fits together but not get into details about every little thing.
  • New reference page that describes syntax in detail and gives examples.
    • A focus would be to make everything very findable, so some clear grouping would be essential.

I think this would align nicely with splitting user and reference content too.

@pditommaso
Copy link
Member

Please keep PR small

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman changed the title Update docs and tests with strict syntax Document formal script syntax Sep 30, 2024
Copy link
Contributor

@christopher-hakkaart christopher-hakkaart left a comment

Choose a reason for hiding this comment

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

Adding a few comments for visibility now.

docs/module.md Show resolved Hide resolved
docs/module.md Outdated Show resolved Hide resolved
docs/module.md Outdated Show resolved Hide resolved
docs/module.md Show resolved Hide resolved
docs/module.md Outdated Show resolved Hide resolved
docs/process.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/module.md Show resolved Hide resolved
@bentsherman
Copy link
Member Author

Okay I think I found a good scope for this PR:

  • Add a new syntax reference page which describes the entire Nextflow syntax
  • Cleanup existing pages now that they don't have to cover syntax so much, just link to syntax page instead
  • Add section to scripts page on how to transition from code snippet to full pipeline script with processes, workflows, etc
  • Update code snippets to comply with formal syntax
  • Remove unnecessary references to Groovy in favor of Nextflow

Some items for future PRs:

  • Update all e2e tests to comply with formal syntax
  • Expand standard library docs to cover things like List, Map, Set APIs
  • Continue to massage the pages under Developing Pipelines to be more focused

Thanks @christopher-hakkaart for the comments, I will address them shortly

@bentsherman bentsherman marked this pull request as ready for review October 1, 2024 00:07
@bentsherman bentsherman requested a review from a team as a code owner October 1, 2024 00:07
bentsherman and others added 4 commits September 30, 2024 19:18
Co-authored-by: Christopher Hakkaart <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Co-authored-by: Christopher Hakkaart <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Co-authored-by: Christopher Hakkaart <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Copy link
Contributor

@christopher-hakkaart christopher-hakkaart left a comment

Choose a reason for hiding this comment

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

Looking good. I've reviewed every md page - except the new syntax page.

I like the direction of the syntax page but I find some of the descriptions too short. E.g., comparing the comments section here and the groovy sytnax page. What do you think about adding more context to some of these? Do you think it would be too repetitive?

docs/developer/plugins.md Outdated Show resolved Hide resolved
docs/dsl1.md Outdated Show resolved Hide resolved
docs/module.md Show resolved Hide resolved
docs/overview.md Outdated Show resolved Hide resolved
docs/process.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

Had a quick run through of just the new syntax.md file and dropped a few first impressions.

This is great!

docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
bentsherman and others added 3 commits October 1, 2024 08:59
Co-authored-by: Christopher Hakkaart <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@bentsherman bentsherman added this to the 24.10.0 milestone Oct 6, 2024
bentsherman and others added 3 commits October 8, 2024 08:21
Signed-off-by: Christopher Hakkaart <[email protected]>
This reverts commit 597e06e.

Signed-off-by: Christopher Hakkaart <[email protected]>
Copy link
Contributor

@christopher-hakkaart christopher-hakkaart left a comment

Choose a reason for hiding this comment

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

This is looking awesome.

I've added some suggestions to turn passive into active voice and split some sentences. I don't see any of these as blockers but opportunities to improve clarity. If you're happy with these I think it's good to go.

docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/reference/syntax.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/script.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/config.md Outdated Show resolved Hide resolved
docs/process.md Outdated Show resolved Hide resolved
Copy link
Contributor

@christopher-hakkaart christopher-hakkaart left a comment

Choose a reason for hiding this comment

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

Thanks for adding my suggestions. The docs look good to me.

docs/config.md Outdated Show resolved Hide resolved
Signed-off-by: Ben Sherman <[email protected]>
This reverts commit 48ac8b5.

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Ben Sherman <[email protected]>
@pditommaso pditommaso merged commit a48ac58 into master Oct 23, 2024
5 checks passed
@pditommaso pditommaso deleted the docs-strict-syntax branch October 23, 2024 16:48
@pditommaso
Copy link
Member

I've left out the mention to deprecation because deserver further discussion 👉 3f6f377

alberto-miranda pushed a commit to alberto-miranda/nextflow that referenced this pull request Nov 19, 2024

Signed-off-by: Ben Sherman <[email protected]>
Signed-off-by: Christopher Hakkaart <[email protected]>
Signed-off-by: Paolo Di Tommaso <[email protected]>
Co-authored-by: Christopher Hakkaart <[email protected]>
Co-authored-by: Paolo Di Tommaso <[email protected]>
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.

4 participants