Skip to content

Get linting to pass#13

Merged
nickp60 merged 11 commits intonickp60:devfrom
vinisalazar:dev
Mar 6, 2026
Merged

Get linting to pass#13
nickp60 merged 11 commits intonickp60:devfrom
vinisalazar:dev

Conversation

@vinisalazar
Copy link

Hi @nickp60,

Please excuse the massive PR. These are several small fixes to get the linting GitHub Actions to pass on the CI environment. I also updated the .devcontainer directory to the latest version used by nf-core/tools.

Could you check if GitHub Actions are enabled in this repo? This way we can make sure the linting action indeed passes.

Best,
Vini

@vinisalazar
Copy link
Author

vinisalazar commented Mar 3, 2026

Still getting an error in tests (see vinisalazar#1), presumably due to the humann module downloading a container image during runtime.

@nickp60
Copy link
Owner

nickp60 commented Mar 3, 2026

Hi @vinisalazar ! Thanks for putting all this together! I've now enabled the actions, hopefully those should kick in with your next commit. Overall it looks amazing, two minor requests:

  • could you run a git rebase to clean up the history a bit since this has so many changes, some of which overlap with the current dev branch?
  • In some places you've replaced branch names main with master -- could you revert those back to main?

I'll take a look at the container issue, but I think it might have to do with my module lacking -stub mode, which I can fix!

@vinisalazar
Copy link
Author

vinisalazar commented Mar 4, 2026

Hey @nickp60, thanks for that

  • Applied a git rebase using the dev branch from this fork. Let me know if you'd like to do anything else, eg. squash the commits further
  • Pretty sure that change was due to a lint error. I will revert it and check the status, it might be the case of doing something to ignore that lint check. Edit: 623ed10

@vinisalazar
Copy link
Author

@nickp60 I had a go with 235d870, but there's still container images being downloaded at runtime.

I'm presuming is the container mapping logic in the humann/base.nf module which is only evaluated at runtime

@nickp60
Copy link
Owner

nickp60 commented Mar 5, 2026

Hi Vini, can you retry the rebase following the scheme laid out here? It looks like at some point the --force-with-lease arg needed to be used: I'm still seeing the PR's diff showing "changes" that already exist in dev.

Was the build_container directory added by the linter? I haven't see that in other nf-core pipelines, I'd assume that container would already be on some registry, rather than having to be built fresh each time.

Thanks again, sorry for the git spaghetti! And sorry for the delayed reply

  - Add missing schema to allOf
  - Run 'nf-core pipelines lint --fix files_unchanged' to fix linting
  - This will be reverted back to the canonical repo after datasets are merged
  - Add/edit params in nextflow.config
  - Remove outdated params from nextflow_schema.json
  - Remove template string from humann_v4 test
  - Apply pre-commit
  - Fix formatting
  - Add introduction
  - Fix steps list
  - Add myself to contributors
  Match nf-core/tools@3.5.2
  - Don't use references to 'master' branch
  - Add new stub tests to humann/regroup and humann/renorm modules
  - Run test profile in stub mode
@vinisalazar
Copy link
Author

Hi Vini, can you retry the rebase following the scheme laid out here? It looks like at some point the --force-with-lease arg needed to be used: I'm still seeing the PR's diff showing "changes" that already exist in dev.

Hi Nick, just tried that, but not sure if it did any good. Could you be more specific as to what changes exactly already exist? I suspect there is some duplicated work on your fork, which I'm trying to sync by rebasing and it's causing conflicts.

Was the build_container directory added by the linter? I haven't see that in other nf-core pipelines, I'd assume that container would already be on some registry, rather than having to be built fresh each time.

Thanks, I think that came when updating .devcontainer setup, I removed it from the branch.

@nickp60
Copy link
Owner

nickp60 commented Mar 6, 2026

Hi Vini, that appears to have fixed it, thanks! merging now so we can move on to other things, thanks again!

@nickp60 nickp60 merged commit daec7c0 into nickp60:dev Mar 6, 2026
1 of 3 checks passed
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.

2 participants