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

[create] Option to set branch names #1961

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from

Conversation

fabianegli
Copy link
Contributor

@fabianegli fabianegli commented Oct 20, 2022

Follow up PR to #1959

Closes #1960

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

@fabianegli
Copy link
Contributor Author

Docs are missing. I would welcome contributions.

@fabianegli
Copy link
Contributor Author

@mashehu This could now probably be turned into an interactive questionaire?

@codecov
Copy link

codecov bot commented Oct 20, 2022

Codecov Report

Merging #1961 (b0f05c3) into dev (f3b5d1e) will decrease coverage by 10.85%.
The diff coverage is 100.00%.

❗ Current head b0f05c3 differs from pull request most recent head 5526752. Consider uploading reports for the commit 5526752 to get more accurate results

@@             Coverage Diff             @@
##              dev    #1961       +/-   ##
===========================================
- Coverage   71.95%   61.11%   -10.85%     
===========================================
  Files          77       37       -40     
  Lines        8363     4678     -3685     
===========================================
- Hits         6018     2859     -3159     
+ Misses       2345     1819      -526     
Impacted Files Coverage Δ
nf_core/__main__.py 53.48% <100.00%> (-5.33%) ⬇️

... and 75 files with indirect coverage changes

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 Oct 27, 2022

My main question with this (and also #1959) is why do we want the default branch name to be variable? Surely we want it to always be master? We can change this to main later or whatever, but what we don't want is for different people to have different branch names.

Is it not better to just manually set the branch name after init with hardcoded values instead of passing variables around and letting defaults / CLI options set this, if having anything other than master will break everything else downstream?

@fabianegli
Copy link
Contributor Author

fabianegli commented Oct 27, 2022

I agree that hardcoding master would work for the functionality of the status quo and it is simpler - so more desirable.

My thinking behind the parametrization was that it will make it easier to try and test things during the migration from master to main. Once the migration is done, the branch customization can/should be stripped out of the code base again. Logging a warning when branch customization is used might be indeed a good idea if it is decided to keep it for the specific task of migrating to main.

@kenibrewer
Copy link
Member

A reason to make the default branch name accessible now is that it would allow for people who use nf-core tools to create non-nfcore pipelines to begin using main as the default branch now. I would love to see this change merged now and would be happy to contribute some docs writing if that's holding things up.

@ewels ewels changed the title Expose default branch name setting for pipeline creation in CLI [create] Option to set branch names Jun 27, 2023
@ewels ewels added the command line tools Anything to do with the cli interfaces label Jun 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
command line tools Anything to do with the cli interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants