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

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

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from

Conversation

jfy133
Copy link
Member

@jfy133 jfy133 commented Jan 17, 2025

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

Copy link
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
Member Author

jfy133 commented Jan 17, 2025

Fine with me (although arguably a very small change)

Copy link
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 :)

CHANGELOG.md Outdated Show resolved Hide resolved
@awgymer
Copy link
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
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

CHANGELOG.md Outdated Show resolved Hide resolved
@awgymer
Copy link
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
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
Contributor

awgymer commented Jan 22, 2025

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

@jfy133
Copy link
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
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.

6 participants