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

be compliant with black defaults in the pipeline template #1788

Merged
merged 2 commits into from
Sep 1, 2022

Conversation

fabianegli
Copy link
Contributor

@fabianegli fabianegli commented Aug 30, 2022

Fixes https://nfcore.slack.com/archives/CE5LG7WMB/p1661868498109569

This PR prevents the need for a pyproject.toml file in the pipeline template.

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

@codecov
Copy link

codecov bot commented Aug 30, 2022

Codecov Report

Merging #1788 (9d30b2b) into dev (b48be11) will decrease coverage by 0.09%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##              dev    #1788      +/-   ##
==========================================
- Coverage   69.33%   69.23%   -0.10%     
==========================================
  Files          59       59              
  Lines        7148     7148              
==========================================
- Hits         4956     4949       -7     
- Misses       2192     2199       +7     
Impacted Files Coverage Δ
nf_core/create.py 62.35% <0.00%> (-2.67%) ⬇️

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

@ewels
Copy link
Member

ewels commented Aug 30, 2022

Thanks for this! The downside of is that the check_samplesheet.py script is being heavily edited between basically every release at the moment. So although it fixes the error for now I feel like it'll just be a matter of time until the same thing happens again..

I wonder if getting the linting config to match nf-core/tools as in #1789 is more sustainable..

@fabianegli
Copy link
Contributor Author

fabianegli commented Aug 30, 2022

I agree, this is a minimalistic fix for the immediate issue, not a preventive measure.

I think we need to think a bit more about preventing the issue in the future. This issue is a symptom of a broader problem which is that this script come snot in the form of standard Python packaging. It lacks not only the code formatting settings, but it also lacks tests and this is a big issue which should be fixed somehow. Either with a separate distribution, making it part of nf-core or adding tests to the bin directory and setting up pytest for this as well. The latter solution would then also affect the pyproject.toml.

My aversion against the pyproject.toml solution in the pipeline template is based in the fact that this will change the constraints for linting in the whole pipeline and this might as well lead to unexpected issues.

@fabianegli
Copy link
Contributor Author

PS: This PR does not prevent the addition of a pyproject.toml.

@ewels
Copy link
Member

ewels commented Aug 30, 2022

Yes all good points 👍🏻 Maybe worth mentioning that this python script will very likely be replaced / removed in the near future (see #sample-sheets Slack channel) so I don't think it's worth putting a lot of work into it at this point.

@fabianegli
Copy link
Contributor Author

After skimming through the #sample-sheet channel, I agree, there is no point in putting much effort in this script.

I think the changes in this PR make the code more readable and can be merged, but if this is not desired this PR should be closed.

Copy link
Member

@ewels ewels left a comment

Choose a reason for hiding this comment

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

LGTM. Leaving to @mirpedrol or @mashehu to merge as they have the bigger picture.

@mashehu mashehu mentioned this pull request Sep 1, 2022
4 tasks
@mirpedrol mirpedrol merged commit ee6b104 into nf-core:dev Sep 1, 2022
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