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.

Sign up for GitHub

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

Jump to bottom

Remove get process and software name functions from versions.yml creation #1326

Closed
wants to merge 3 commits into from
Closed

Remove get process and software name functions from versions.yml creation #1326

wants to merge 3 commits into from

Conversation

JoseEspinosa
Copy link
Member

Remove custom functions getProcessName and getSoftwareName and instead use the template to directly populate the process name and the software name within the versions.yml code.

Closes #1284

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

@JoseEspinosa JoseEspinosa added DSL2 v2.0 WIP Work in progress labels Nov 23, 2021
@JoseEspinosa JoseEspinosa self-assigned this Nov 23, 2021
@JoseEspinosa JoseEspinosa changed the title Remove get process and software name functions Remove get process and software name functions from versions.yml creation Nov 23, 2021
grst
grst reviewed Nov 23, 2021
View reviewed changes
Copy link
Member

@grst grst left a comment

Choose a reason for hiding this comment

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

There are a few other things that need to be updated in the module pipeline, e.g

  • get rid of functions.nf
  • get rid of publish dir

but is that supposed to be part of this PR?

nf_core/module-template/modules/main.nf Outdated
@@ -79,8 +79,8 @@ process {{ tool_name_underscore|upper }} {
$bam

cat <<-END_VERSIONS > versions.yml
${getProcessName(task.process)}:
${getSoftwareName(task.process)}: \$( samtools --version 2>&1 | sed 's/^.*samtools //; s/Using.*\$//' )
{{ tool_name_underscore|upper }}:
Copy link
Member

Choose a reason for hiding this comment

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

Didn't we go for ${task.process} here?

Copy link
Member Author

@JoseEspinosa JoseEspinosa Nov 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, forgot to push the change 😢

@JoseEspinosa
Copy link
Member Author

JoseEspinosa commented Nov 23, 2021 •
edited
Loading

There are a few other things that need to be updated in the module pipeline, e.g

* get rid of functions.nf

* get rid of publish dir

but is that supposed to be part of this PR?

I am addressing all the other changes in another PR. At the beginning, I was keeping separate the update of the modules template and the pipeline template, but they have some dependencies, so I went for a single draft here: #1327
I will add the things we have just discussed and ping you there.

@JoseEspinosa
Copy link
Member Author

JoseEspinosa commented Nov 23, 2021 •
edited
Loading

Thinking again, might be worth closing this draft and adding everything in the same PR? 🤔

@grst
Copy link
Member

grst commented Nov 23, 2021

Yeah, let's do that 🚀

@JoseEspinosa JoseEspinosa reopened this Nov 23, 2021
@JoseEspinosa
Copy link
Member Author

These changes are addressed in #1327 and thus, this draft PR has been closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@grst grst grst left review comments

Assignees

@JoseEspinosa JoseEspinosa

Labels
WIP Work in progress
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@JoseEspinosa @grst