https://nf-co.re logo
Join Slack
Powered by
Hello everyone, I have a question about PR size ....
# infrastructure
h
Hello everyone, I have a question about PR size . i am trying to integrate sourmash in taxprofiler and my PR is flagged by the size labeler: "`message_if_xl: This PR exceeds the recommended size of 1000 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.` Any thoughts about this? is this just a warning or should i split it up?
e
warning mainly, but reviewers are always happier to trade smaller reviews 😄
easier to bargain
h
thanks for the answer, Isnt that big though. can i bypass it somehow (i noticed limit is set in .github/workflows/labeler.yml) or just let reviewers know?
e
wait, does it actually stop you from opening the PR? If no, go ahead. Curious, how many files does that entail?
h
Yeah it's blocking the PR as a failing check. 42 files changed. labeler log gives error 
cant write to the PR from my fork
github.com/nf-core/taxprofiler/…/110279198010?pr=771
e
Weird, that's not that much indeed.. didn't even know there was a lines limitation..would turn the rule off then, and back on after the merge (if it's part of the template)
p
Is this something @James Fellows Yates set up?
I remember talking about the idea a while back. My understanding was a warning + perhaps a label on the PR, not for it to be blocking though.
Ah:
Copy code
Adding size label: size/xl
curl: (22) The requested URL returned error: 403
GitHub API request failed. If access was denied, add this to the job in your GitHub Actions workflow:
  permissions:
    pull-requests: write
For pull requests from forks, use pull_request_target to get a writable token.
Yes, from @James Fellows Yates: github.com/nf-core/taxprofiler/pull/681
j
Yeah we were experimenting with it to see if it helps with attracting reviews
But I think we can probably remove it as I didn't get any feedback
p
Not sure why it's failing now, other PRs from forks etc. look fine
Might have helped if the labels were coloured? They're quite hard to distinguish
e
That's 
rich
, coming from you
🥁 2
😆 2
😄 2
h
thanks for the answers @Phil Ewels, @Evangelos Karatzas. @James Fellows Yates For now shall i split it or you considering removing it??
p
If you get an approval I can force-merge
But even with failing tests it's not ready current as there is no PR approval
h
ok great thanks! i will wait for the final comments/thougths from @Sofia Stamouli & @James Fellows Yates
Open in Slack
PreviousNext
Powered by