https://nf-co.re logo
Join Slack
Powered by
Anyone wants to trade <https://github.com/nf-core/...
# release-review-trading
d
Anyone wants to trade https://github.com/nf-core/ampliseq/pull/1014? Second review needed for the release šŸ˜„
āœ… 1
merged 1
n
šŸ‘€ 1
d
Alright, will start today
n
I will also try to start today, but maybe I will only get around tomorrow..
d
Hm, I had a first look to get an overview, but neither linting nor the sole test are passing. Are you sure it makes sense to review that already?
n
Probably better if i fix things first
And update the template..
d
Noooo
Dont update the template, dont make the PR larger
Imho try to fix what can be fixed with minimal changes, make follow-up PRs instead of piling on!
šŸ™Œ 1
Finished with review. I would be fine to check again. But please do not make sweeping changes that could be separated into other PRs!
n
Thanks a lot. In that case, I will not update the template. I have an issue with keeping PRs focused, so thanks for the pointer
Ok, so I think I adressed everything you pointed out. I think there is an issue with pipeline linting, that relates to the fact that this PR is on template 3.5.1, since locally with tools 4.0.2 linting works. Should I merge the template into dev and rebase?
d
The issue with linting seems to be that nextflow 26.04.3 is used, which fails t parse the pipeline code, because its incompatible. Nothing to be done expect a template update, because that should help here. Again, I wouldnt do that in that PR.
I think the test fails because the pipeline uses nf-schema@2.5.1, an update might help, as in https://github.com/nf-core/ampliseq/pull/1006.
For both issues, I would first confirm that my assumptions are right in the appropriate slack channels though.
Depending on the extend of the change needed to make tests/linting pass, I would do it (nf-schema) or not do it (template update) in that PR.
n
Ok, thanks I think I managed to solve the things that stop 
nextflow lint
but there is one thing that I do not get: I am doing this in one of my files:
Copy code
script:
...
    def groupBuilder = new groovy.yaml.YamlBuilder()
    groupBuilder(groups)
and lint complains: ``` Error modules/local/report/main.nf635: 
groupBuilder
is not defined ā”‚ 63 | groupBuilder(groups) ā•° | ^^^^^^^^^^^^ ` ``
d
No idea unfortunately
n
Ok šŸ˜• I will see how others solve that
d
If you can still solve linting & test, or if not feasible in that PR then state that you will fix it in an upcoming PR and I'll give you an ok
n
I am trying to fix things, I will ping you once I give up šŸ˜‰
šŸ˜„ 1
I fixed the linting, the tests are running but longstitch is so annoying in tests that I am considering removal from the pipeline. I hope I have correctly ignored the files that seem to be randomly changing, let me know if there are any blockers. This contains now approx 50% of the template merge to get things running, so at least that should then not confluct too much
@Daniel Straub its all done and the template is merged, good to go from your side?
2 Views
Open in Slack
PreviousNext
Powered by