https://nf-co.re logo
Join Slack
Powered by
Hi all! this will be my first release <https://git...
# release-review-trading
o
Hi all! this will be my first release https://github.com/nf-core/bacmodel/pull/1
d
I can have a look later today. Will have one for #C02FC6VFQG1 shortly to trade with.
❤️ 1
o
Thanks!
m
created a pseudo PR to make reviewing easier (just shows changes compared to the template): https://github.com/nf-core/bacmodel/pull/5
after creating that, saw that there are some testing artifacts in the repo, removed with this PR https://github.com/nf-core/bacmodel/pull/7
d
I saw those too. Are you reviewing now? I.e. should I wait a moment?
(Is there a smart way to transfer comments from one PR to another? Should I perhaps start by closing my first review right now?)
m
doesn't matter where the comments are, the changes will come in at both. continue wherever you prefer. will not be able to do any reviews before next week probably, so no reason to hold off 🙂
d
It sounded like you did me a service by creating the two new, so maybe I just close this one and continue on number 7. @Olga Brovkina, you'll get a partial review of number 1 now.
👍🏻 1
👍 1
Or should I or @Olga Brovkina just merge number 7 and then I continue with #6?
I'm confused
m
#6 is a template udpate. do you mean #5?
d
I don't know. I'd prefer something that doesn't have close to 3,000 files.
m
yes, let's merge the clean-up PR and then you should have an easier review process
d
I do that?
m
if you give a review on it, i can do it and take the blame 😁
👍 1
125 changed files in the pseudo PR. a bit easier to handle 😅
d
Indeed -- thanks!
So, the pseudo PR is the one I review? Or number 1?
m
pseudo PR should be easier because you don't get template updates also shown as changes
👍 1
o
Thanks for sorting this out and for taking the time to review the pipeline ❤️
m
also just saw that for some reason the github actions where not yet enabled on this repo. so now you should get some linting errors etc
👍 2
d
First round ready for you @Olga Brovkina!
👍 1
o
Hi @Daniel Lundin and @mashehu, I have addressed the comments from the first round of review. In addition, I contacted the gapseq developers to review the corresponding part of the pipeline and received further feedback from them, which also took some time to implement. Could I kindly ask for a second round of review ? 😊
d
Will do. Tomorrow
❤️ 1
m
did you see my comments on https://github.com/nf-core/bacmodel/pull/5? If so please resolve them
👍 1
d
Which of the PRs should I look at once I get to this tomorrow?
m
They have the same updates
👍 1
d
It looks like you didn't resolve my comments either. Please do and let me know. Best of course if you resolve both mine and Matthias'.
o
Hi everyone! And sorry—it was indeed a false start. 😅 I've now gone through all the comments on the current PR and addressed them to the best of my ability. Please let me know if there's anything else I should take care of or if you have any additional feedback.
d
Will have a look today.
❤️ 1
Made some comments. Only one thing that's important before I can approve, the rest are up to you.
👍 1
o
Thank you!
I met this nf-core runners issue on the GitHub side 😓 I will wait for Matthias return from vacation
Hi @Daniel Lundin ! I adjusted the pipeline according to your comments. And the nf-core runners worked as usual. So, please, have a look 😊
d
I'm at a conference, but will do as soon as I can. Remind me Thursday if you haven't seen anything.
👌 1
I just posted a review of the pseudo-PR. I'm a bit confused as I say in the review message, so let's see if this was useful. (Since this one isn't supposed to be merged, I guess you need an OK on another? I didn't give you an OK now anyway because I have (at least) one unanswered question, but for the next round.)
2 Views
Open in Slack
PreviousNext
Powered by