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

Dev #119

Closed
wants to merge 16 commits into from
Closed

Dev #119

wants to merge 16 commits into from

Conversation

lborcard
Copy link

Add flye

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the nf-core/bacass branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

@github-actions GitHub Actions
Copy link

github-actions bot commented Mar 20, 2024 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit df0eb71

+| ✅ 176 tests passed       |+
#| ❔   2 tests were ignored |#
!| ❗   2 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in main.nf: Optionally add in-text citation tools to this list.
  • pipeline_todos - TODO string in main.nf: Optionally add bibliographic entries to this list.

❔ Tests ignored:

✅ Tests passed:

Run details

  • nf-core/tools version 2.13.1
  • Run at 2024-03-21 10:09:42

Daniel-VM
Daniel-VM reviewed Mar 21, 2024
View reviewed changes
Copy link
Contributor

@Daniel-VM Daniel-VM left a comment

Choose a reason for hiding this comment

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

Please, add citations here and here too.

CHANGELOG.md Outdated
@@ -31,6 +31,7 @@ This version merges the nf-core template updates of v2.9 and v2.10, and updates

### `Added`

- [#116](https://github.com/nf-core/bacass/pull/116) - Added Flye as an alternative assembler for long reads
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to be added in section 2.2.0dev not in v2.1.0.

@Daniel-VM
Copy link
Contributor

It would be great to have a test profile for flye in conf/ . Ideally this test should be executed by git CI tests. See #106 (comment)

@Daniel-VM
Copy link
Contributor

It appears that your changes are pulling lines from master. In other words, it seems like you're pulling changes from master, merging them into your dev branch, and then pushing your dev branch into nf-core/bacass:dev. This is risky because we might overwrite newer lines in the dev branch.

My advice is to:
1- fork github nf-core/bacas to your github ("origin")
2- pull origin to your working directory in your local computer.
3- checkout to dev and start making changes in this dev.

once completed:
4- push your local dev branch to your origin dev
5- PR form your origin dev to nf-core/bacass:dev.

See sections Contribution overview, testing, pipeline tests in nf-core guidelines about contributing to an existing pipeline.

@lborcard
Update CHANGELOG.md
e12b034 
Moved changes to v2.2.0dev
@lborcard
Update nextflow.config
66acb5a 
added dragonflye in the list of assembler available
@lborcard
Update nextflow_schema.json
90d0a3c 
changed path of assets
@Daniel-VM
Copy link
Contributor

Careful, I have seen that you have created another branch directly into nf-core/bacass original repo. this is not the way we have to follow. New branches needs to be created in your forked repo in order to make PRs like this: forked_repo:dev to nf-core_repo:dev.

@lborcard
Copy link
Author

It appears that your changes are pulling lines from master. In other words, it seems like you're pulling changes from master, merging them into your dev branch, and then pushing your dev branch into nf-core/bacass:dev. This is risky because we might overwrite newer lines in the dev branch.

My advice is to: 1- fork github nf-core/bacas to your github ("origin") 2- pull origin to your working directory in your local computer. 3- checkout to dev and start making changes in this dev.

once completed: 4- push your local dev branch to your origin dev 5- PR form your origin dev to nf-core/bacass:dev.

See sections Contribution overview, testing, pipeline tests in nf-core guidelines about contributing to an existing pipeline.

Careful, I have seen that you have created another branch directly into nf-core/bacass original repo. this is not the way we have to follow. New branches needs to be created in your forked repo in order to make PRs like this: forked_repo:dev to nf-core_repo:dev.

This problem was fixed, I am now working from a fork of the bacass:dev so I wonder if there are issues from a previous version, before I changed to my fork

@lborcard
Copy link
Author

I don't see anything coming from the master branch, I am probably missing something.

Careful, I have seen that you have created another branch directly into nf-core/bacass original repo. this is not the way we have to follow. New branches needs to be created in your forked repo in order to make PRs like this: forked_repo:dev to nf-core_repo:dev.

Yes this was my initial mistake, I am not using this branch anymore. Sorry about the inconvenience.

@lborcard
Copy link
Author

Thanks a lot for the help, it seems that a lot of the problem come from the fact that I initially used the master branch to work on the pipeline. I will now close this add the new suggestions.

@lborcard lborcard closed this Mar 21, 2024
@lborcard
Copy link
Author

There are remaining issues from the previous branch and additional items that need to be added.

@Daniel-VM
Copy link
Contributor

Exactly 😅. Yes. It would be easier to use a fresh dev branch before adding changes.

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

@Daniel-VM Daniel-VM Daniel-VM left review comments

@nf-core-bot nf-core-bot Awaiting requested review from nf-core-bot

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
@lborcard @Daniel-VM @d4straub @mashehu