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

Change sed regex for version reporting in pigz modules #6878

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

ljwharbers
Copy link

@ljwharbers ljwharbers commented Oct 28, 2024

Missing backslash in pigz uncompress stub results in unknown recognition error type: groovyjarjarantlr4.v4.runtime.LexerNoViableAltException

Simplified sed regex to circumvent the missing/added trailing backslash

PR checklist

Closes #6877

  • [ x ] This comment contains a description of changes (with reason).

Copy link
Contributor

@fellen31 fellen31 left a comment

Choose a reason for hiding this comment

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

Could you add a stub test to this module?

Also, make sure to ask to be added to the nf-core GitHub organisation so you can run the GitHub actions :)

@ljwharbers
Copy link
Author

@fellen31 Thanks for the reply.
Diving a bit deeper into the sed logic I believe that it wasn't giving the expected result.

I'm still new to contributing to nf-core, but I'm more than happy to help with adding tests. Could you either point me to some examples of how to add tests for this?

@fellen31
Copy link
Contributor

I'm still new to contributing to nf-core, but I'm more than happy to help with adding tests. Could you either point me to some examples of how to add tests for this?

Sure. You would be adding another test identical the current one but with options "-stub" (also add stub to the name of the test). Afterwards run e.g. nf-test test modules/nf-core/pigz/uncompress/ --profile docker --update-snapshot to generate a new snapshot. This will test the stub part of the module, and should catch errors like the one you reported.

For a simple example: https://github.com/nf-core/modules/blob/3abde3de0970781e8e9aeec3cfeed759008a6ef4/modules/nf-core/unzip/tests/main.nf.test

@ljwharbers ljwharbers changed the title added missing backslash in pigz uncompress stub Change sed regex for version reporting in pigz modules Oct 30, 2024
@ljwharbers ljwharbers marked this pull request as draft October 30, 2024 10:01
@ljwharbers ljwharbers marked this pull request as ready for review October 30, 2024 10:51
@ljwharbers
Copy link
Author

@fellen31 I think it's all in order now. Except for one linting test failing for a different module. Not sure why/how that get's included in these tests.

Is the typical procedure that I would also fix the linting for that module or is this typically still merged because I did not make any changes to those module files?

Thanks again for pointing me in the right direction!

@fellen31
Copy link
Contributor

fellen31 commented Oct 30, 2024

No you don't need to fix the others. When you update this branch with the latest master, the other test should not be removed from the list of checks.

There was a problem with GitHub Actions earlier today, maybe something is still not working correctly.

@fellen31
Copy link
Contributor

fellen31 commented Oct 30, 2024

Or actually, in this case it's because PIGZ_COMPRESS is being used in toffee/irmsd tests. It looks like it should be a simple fix (updating meta.yml to match main.nf), so perhaps you could try to fix in this PR, so we can merge with all green checkmarks.

@ljwharbers ljwharbers marked this pull request as draft October 30, 2024 11:57
@ljwharbers
Copy link
Author

Looking into tcoffee/irmsd, it needs the added types and descriptions in the yaml file. However, while I can 'guess' the types based on the nextflow code, I have no idea what the descriptions should be.

So perhaps this is better that someone adds it who actually uses this.

@ljwharbers ljwharbers marked this pull request as ready for review October 30, 2024 14:21
Copy link
Contributor

@fellen31 fellen31 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 the stub test, looks good to me! 👍

@fellen31
Copy link
Contributor

Looking into tcoffee/irmsd, it needs the added types and descriptions in the yaml file. However, while I can 'guess' the types based on the nextflow code, I have no idea what the descriptions should be.

So perhaps this is better that someone adds it who actually uses this.

Perhaps guessing a bit is ok here, or maybe @luisas could help. I think the linting has been updated to include more checks since that module was added.

@ljwharbers
Copy link
Author

Alright, I've added the types and some general descriptions. Feel free to make adjustments @luisas

@SPPearce SPPearce enabled auto-merge November 26, 2024 19:49
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.

pigz uncompress missing backslash
3 participants