Skip to content

Remove args stub from module template to satisfy language server#3403

Merged
jfy133 merged 6 commits intodevfrom
remove-args-stub
Jun 19, 2025
Merged

Remove args stub from module template to satisfy language server#3403
jfy133 merged 6 commits intodevfrom
remove-args-stub

Conversation

@jfy133
Copy link
Copy Markdown
Member

@jfy133 jfy133 commented Jan 17, 2025 •
edited
Loading

I don't know if there is a linting check for this though...

People can add it back in if needed within the stub

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

mirpedrol
mirpedrol requested changes Jan 17, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Let's wait to merge this until v3.2.0, so we don't have bigger changes in a patch release. I am blocking the merge but changes LGTM 🙂

@mirpedrol mirpedrol added this to the 3.2 milestone Jan 17, 2025
@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jan 17, 2025

Fine with me (although arguably a very small change)

mirpedrol
mirpedrol approved these changes Jan 20, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

ok to merge now :)

mirpedrol
mirpedrol reviewed Jan 20, 2025
View reviewed changes
Comment thread CHANGELOG.md Outdated
@awgymer
Copy link
Copy Markdown
Contributor

awgymer commented Jan 21, 2025

Would it be potentially better to do echo $args in the stub section?

This has a couple of benefits over dropping the $args:

  • maintains consistency between script and stub section logic
  • adds a simplistic level of testing that args are behaving as expected
  • keeps the language server happy

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jan 21, 2025

I had thought the same thing as one option @awgymer , but was outvoted

https://nfcore.slack.com/archives/C043UU89KKQ/p1737099337825629

jfy133
jfy133 commented Jan 21, 2025
View reviewed changes
Comment thread CHANGELOG.md Outdated
@awgymer
Copy link
Copy Markdown
Contributor

awgymer commented Jan 21, 2025

That is a shame. It seems like people have a different view of the purpose of stub to me.

I wish it would behave more strictly as a "ran the pipeline as if it were real except never actually executed the script" mode.

I have had this open issue for some time in nextflow because sometimes you have more complex logic for e.g. determining if you should add a --reference ${fasta} arg into your script, and that logic can run fine and quickly in stub mode but you currently can't test it in-place using stub (you have to duplicate the entire logic from script, which I still think is worth doing but I guess others do not 🥲)

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jan 21, 2025

Bring it up on the slack channel! I don't mind changing the PR myself :)

@awgymer
Copy link
Copy Markdown
Contributor

awgymer commented Jan 22, 2025

Are normies allowed to join and comment in #team-maintainers 🤔

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jan 22, 2025

Are normies allowed to join and comment in #team-maintainers 🤔

Yes :)

@mirpedrol mirpedrol modified the milestones: 3.2, 3.3.0 Jan 27, 2025
@mirpedrol
Copy link
Copy Markdown
Member

mirpedrol commented Mar 11, 2025

🧹 spring cleaning message 🌷

What is the state of this PR? did #team-maintainers reach a consensus?

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Mar 11, 2025

No, we argued about it 🤣, maybe you could broach the topic again on slack with a vote @mirpedrol

mirpedrol
mirpedrol requested changes Mar 12, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Looks like we have a decision https://nfcore.slack.com/archives/C043UU89KKQ/p1741767322271249?thread_ts=1737099337.825629&cid=C043UU89KKQ

So this PR should be modified to keep the definition of args and add an echo to the stub. We should also add a TODO comment explaining that these lines can be deleted if the module doesn't use args.

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Mar 12, 2025

@mirpedrol there is a Kita and Hort strike today so can't do much. If you want this done quickly maybe you can make the changes and I can review from my phone

@mirpedrol mirpedrol removed this from the 3.3.0 milestone May 20, 2025
@mirpedrol mirpedrol added this to the 3.4.0 milestone May 20, 2025
@mirpedrol mirpedrol self-requested a review June 19, 2025 10:20
@mirpedrol
Copy link
Copy Markdown
Member

mirpedrol commented Jun 19, 2025

I have made the change, and it's ready to review

mirpedrol
mirpedrol approved these changes Jun 19, 2025
View reviewed changes
Copy link
Copy Markdown
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Approving so my previous review requesting changes doesn't block the PR. But someone else should review since I made the last changes.

@jfy133
Copy link
Copy Markdown
Member Author

jfy133 commented Jun 19, 2025

@mahesh-panchal approves so will merge

@jfy133 jfy133 merged commit 2c43223 into dev Jun 19, 2025
98 checks passed
@jfy133 jfy133 deleted the remove-args-stub branch June 19, 2025 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mirpedrol mirpedrol mirpedrol approved these changes

@mahesh-panchal mahesh-panchal mahesh-panchal approved these changes

@SPPearce SPPearce SPPearce approved these changes

@maxulysse maxulysse Awaiting requested review from maxulysse

@sateeshperi sateeshperi Awaiting requested review from sateeshperi

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

3.3.2

Development

Successfully merging this pull request may close these issues.

6 participants

@jfy133 @awgymer @mirpedrol @mahesh-panchal @SPPearce @nf-core-bot