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

allowing new line before description field #2177

Merged
merged 7 commits into from
Feb 9, 2023

Conversation

mirpedrol
Copy link
Member

Fix a typo that I introduced with #2175

The description field of meta.yml when the flag was not provided was attached to the previous line (without a new line).

We can sneak this into the next PR as it's a tiny change, leaving it here as a reminder.

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

@mashehu
Copy link
Contributor

mashehu commented Feb 7, 2023

I think it is fine to just merge this (and add the PR number as a second reference in the changelog entry)

@mirpedrol
Copy link
Member Author

nf-core/modules#2847 broke some tests 💀

@mashehu
Copy link
Contributor

mashehu commented Feb 7, 2023

why would the rename break the tests? Is something not updated?

@mirpedrol
Copy link
Member Author

We install the subworkflow to an old version, as the module in this subworkflow has the old name when it tries to install it from nf-core/modules it's not found.
I think the module should also be installed to the old version but I'm not sure about how file renaming works when we clone the modules repo.

@mashehu
Copy link
Contributor

mashehu commented Feb 7, 2023

:shacking-fist: @maxulysse
So, updating the subworkflow should fix this?

@mirpedrol
Copy link
Member Author

It might actually be a bug on tools 🤔
We need the outdated subworkflow for this test as we check a change of module, this one is the only subworkflow that I could find with this change :(
Will try to find where are things breaking.

@maxulysse
Copy link
Member

I actually corrected the subworkflow name

@maxulysse
Copy link
Member

nf-core/modules#2847 broke some tests skull

How come? all my tests where ✔️

@mirpedrol
Copy link
Member Author

I think the problem is that we check if the module is available before installing it, and to do this we get the list of available modules from the last nf-core/modules repo version.
If I'm not wrong, this only affects name changes like this one when move from TOOL to TOOL/SUBTOOL, and only when modules are automatically installed from a subworkflow, not when installing them by command line.

@codecov
Copy link

codecov bot commented Feb 8, 2023

Codecov Report

Merging #2177 (cab29c8) into dev (b728841) will increase coverage by 0.04%.
The diff coverage is 84.00%.

@@            Coverage Diff             @@
##              dev    #2177      +/-   ##
==========================================
+ Coverage   71.51%   71.55%   +0.04%     
==========================================
  Files          77       77              
  Lines        8338     8347       +9     
==========================================
+ Hits         5963     5973      +10     
+ Misses       2375     2374       -1     
Impacted Files Coverage Δ
nf_core/components/update.py 81.73% <77.77%> (+0.21%) ⬆️
nf_core/components/install.py 86.95% <100.00%> (ø)
nf_core/modules/modules_repo.py 83.64% <100.00%> (+0.15%) ⬆️
nf_core/modules/modules_json.py 80.26% <0.00%> (+0.03%) ⬆️
nf_core/components/list.py 68.83% <0.00%> (+0.41%) ⬆️
nf_core/sync.py 75.10% <0.00%> (+0.42%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@mirpedrol mirpedrol requested a review from mashehu February 8, 2023 12:11
@mirpedrol mirpedrol mentioned this pull request Feb 9, 2023
4 tasks
@mirpedrol mirpedrol merged commit 825c45b into nf-core:dev Feb 9, 2023
@mirpedrol mirpedrol deleted the small-typo branch February 9, 2023 12:05
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.

3 participants