-
Notifications
You must be signed in to change notification settings - Fork 28
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
Expose the list of (non-)substantive PRs for a given repository. #269
Conversation
@deniak this is the initial WIP I mentioned yesterday, in case it's useful; feel free to ignore/close if not though :) |
…a given repository
e8818a4
to
ea6f5ad
Compare
@dontcallmedom thanks for writing the endpoint. I completed the PR to make use of it and there's now a link to the list of contributors from the list of repositories. |
application/contributors-list.jsx
Outdated
, owner: null | ||
, shortName: null | ||
, substantiveContributors: [] | ||
, nonSubstantiveContributors: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure there is any value in listing non-substantive contributors at this stage, so I would drop it from the UI
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've removed the non-substantive contributors from the UI
application/contributors-list.jsx
Outdated
<td> | ||
<ul> | ||
{(type === substantiveKeys ? st.substantiveContributors : st.nonSubstantiveContributors)[i].prs.map((pr) => { | ||
return <li key={pr}><a href={`${pp}pr/id/${st.owner}/${st.shortName}/${pr}`}>PR #{pr}</a></li> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect it would be more useful to link to the PR itself rather than the related IPR checker page for the PR; unless you had a specific reason to link to the latter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wasn't sure which link would be the most useful but I guess people will be more interested to see the details of the PR instead of the IPR check.
That said, I'm wondering if we should also list the status of the PR (open, close, etc) on the list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
excellent remark! it's clear what we care primarily about is merged pull requests; unfortunately, AFAIR, we only track a open/closed state, and not very reliably either (e.g. if a pull request gets open or closed when ash-nazg is down for any reason, there isn't any systematic sync).
Another thing that would be worth highlighting one way or another is pull requests that got merged despite not having received IPR-validation.
Let's merge and deploy this as is, and follow up in separate issues / PRs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
…given repo