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

fix: Container directive parsing when it is on a single line #2306

Merged
merged 16 commits into from
Jun 15, 2023
Merged

fix: Container directive parsing when it is on a single line #2306

merged 16 commits into from
Jun 15, 2023

Conversation

adamrtalbot
Copy link
Contributor

Fixes parsing of container directives when they don't match standard patterns, e.g. container "hello-world".

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

@adamrtalbot
fix: Parsing of container directive
1143084 
Changes:
  - Strip additional components off container directive prior to parsing
  - Helps parse additional strings that may be present at the front of
    container directive (e.g. 'container ').
  - Should catch more non-standard container directives (of which there
    are many).
@adamrtalbot
CHANGELOG
da476f2
@adamrtalbot
Removed 'removeprefix' which is Py > 3.9 only
c98848a
@codecov
Copy link

codecov bot commented Jun 8, 2023 •
edited
Loading

Codecov Report

Merging #2306 (6c3a218) into dev (11dd7db) will decrease coverage by 0.10%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##              dev    #2306      +/-   ##
==========================================
- Coverage   72.98%   72.89%   -0.10%     
==========================================
  Files          78       78              
  Lines        8756     8760       +4     
==========================================
- Hits         6391     6386       -5     
- Misses       2365     2374       +9
Impacted Files Coverage Δ
nf_core/modules/lint/main_nf.py 68.28% <84.61%> (+0.08%) ⬆️

... and 3 files with indirect coverage changes

@adamrtalbot adamrtalbot requested review from mashehu and mirpedrol and removed request for mashehu June 8, 2023 16:13
@adamrtalbot
Copy link
Contributor Author

8fe15ea is just there to make sure the test runs without a Nextflow failure. Not sure what to do about it really...

@drpatelh
Copy link
Member

drpatelh commented Jun 8, 2023 •
edited
Loading

Looks like this fix hasn't made it into a release yet: nextflow-io/nextflow#3992

Just checked the latest release 23.04.2. Maybe we leave the changes as they are and re-trigger when an edge release comes out. Not sure there is much else we can do. Definitely don't want to pin versions for testing.

@adamrtalbot
Copy link
Contributor Author

agreed, this is just a POC and to test if I broke anything else. Should have put do not merge label on it...

mirpedrol
mirpedrol approved these changes Jun 9, 2023
View reviewed changes
Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

LGTM
I agree with letting this test fail until there is a fix in Nextflow

@drpatelh
Copy link
Member

drpatelh commented Jun 9, 2023 •
edited
Loading

I suspect that there are other modules that are borked too, especially with the new registry syntax. Be good to run nf-core modules lint --all before the release and try and fix as many as possible.

@adamrtalbot adamrtalbot closed this Jun 9, 2023
@adamrtalbot adamrtalbot reopened this Jun 9, 2023
@adamrtalbot
Copy link
Contributor Author

Spotted a problem. Do not merge.

@adamrtalbot adamrtalbot closed this Jun 9, 2023
@adamrtalbot
Copy link
Contributor Author

Fixed, I've changed the logic of the regex quite a bit by parsing the container string as much as possible before just looking for :(tag)$. Let's see how this goes.

@adamrtalbot adamrtalbot reopened this Jun 9, 2023
@adamrtalbot adamrtalbot mentioned this pull request Jun 12, 2023
Merged
4 tasks
@adamrtalbot adamrtalbot mentioned this pull request Jun 13, 2023
Merged
4 tasks
@adamrtalbot adamrtalbot merged commit 59d2309 into nf-core:dev Jun 15, 2023
@adamrtalbot adamrtalbot deleted the fix_parsing_container_spec branch June 15, 2023 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mirpedrol mirpedrol mirpedrol approved these changes

@maxulysse maxulysse maxulysse approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@adamrtalbot @drpatelh @maxulysse @mirpedrol