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

Pseudo-PR: First code review #10

Open
wants to merge 44 commits into
base: first-commit
Choose a base branch
from
Open

Pseudo-PR: First code review #10

wants to merge 44 commits into from

Conversation

likelet
Copy link
Member

@likelet likelet commented Nov 23, 2018

Many thanks to contributing to nf-core/lncpipe!

Please fill in the appropriate checklist below (delete whatever is not relevant). These are the most common things requested on pull requests (PRs).

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 necessary, also make a PR on the nf-core/lncpipe branch on the nf-core/test-datasets repo
  • Ensure the test suite passes (nextflow run . -profile test,docker).
  • Make sure your code lints (nf-core lint .).
  • Documentation in docs is updated
  • CHANGELOG.md is updated
  • README.md is updated

Learn more about contributing: https://github.com/nf-core/lncpipe/tree/master/.github/CONTRIBUTING.md

@likelet
Copy link
Member Author

likelet commented Nov 23, 2018

BTW, I didn't add test data here, as the test data is extremely big (about 2 Gb). I created a test data repo on testdata and stored the data at a local server that allowing remote access. Plz let me know if there is an alternative strategy.

Copy link
Member

@apeltzer apeltzer left a comment

Choose a reason for hiding this comment

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

Some remarks, I'll go over it in more detail later

.travis.yml Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
conf/awsbatch.config Outdated Show resolved Hide resolved
conf/base.config Show resolved Hide resolved
conf/igenomes.config Outdated Show resolved Hide resolved
docs/output.md Show resolved Hide resolved
@apeltzer
Copy link
Member

BTW, I didn't add test data here, as the test data is extremely big (about 2 Gb). I created a test data repo on testdata and stored the data at a local server that allowing remote access. Plz let me know if there is an alternative strategy.

Hosting somewhere else is fine I guess, but please still add a separate branch for lncpipe and state that in a README in that branch so that people know where the test data resides.

Do you have an idea how long your tests are running? Or could you strip the dataset down in size while still running proper tests?

@likelet
Copy link
Member Author

likelet commented Nov 23, 2018

BTW, I didn't add test data here, as the test data is extremely big (about 2 Gb). I created a test data repo on testdata and stored the data at a local server that allowing remote access. Plz let me know if there is an alternative strategy.

Hosting somewhere else is fine I guess, but please still add a separate branch for lncpipe and state that in a README in that branch so that people know where the test data resides.

Do you have an idea how long your tests are running? Or could you strip the dataset down in size while still running proper tests?

Okey, i wll add a branch there. For now , the test data may take less than 10 mins with default setting . I tried stripping the dataset into chrX only for it can produce meanful result which also can promote the understanding to the result.

@likelet
Copy link
Member Author

likelet commented Dec 6, 2018

BTW, I didn't add test data here, as the test data is extremely big (about 2 Gb). I created a test data repo on testdata and stored the data at a local server that allowing remote access. Plz let me know if there is an alternative strategy.

Hosting somewhere else is fine I guess, but please still add a separate branch for lncpipe and state that in a README in that branch so that people know where the test data resides.

Do you have an idea how long your tests are running? Or could you strip the dataset down in size while still running proper tests?

The test dataset has been trimmed into 0.5Gb in total, and hosted on my own server. I also set the url in the .travis.yml , and make it pass the CI building. User can also down the data with wget command. I will write a detailed document it on this.

.travis.yml Show resolved Hide resolved
.travis.yml Show resolved Hide resolved
Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
rename the process name that solved the duplicated processed error by updated Nextflow version from 0.18
@ewels ewels added this to Ready for review in First-release review queue Mar 22, 2019
@ewels ewels moved this from Ready for review to Awaiting changes in First-release review queue Mar 22, 2019
@ewels ewels moved this from Awaiting changes to Ready for review in First-release review queue Dec 9, 2020
@drpatelh drpatelh moved this from Ready for review to In progress in First-release review queue Jan 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

6 participants