6 ms·

I'm all for having THAT discussion. Things that come to mind - - treating PRs as communication and ensuring the person reviewing has the information they need

by mcfunk 8y ago

I'm all for having THAT discussion. Things that come to mind - - treating PRs as communication and ensuring the person reviewing has the information they need to check for what you coded. - Building a culture where discussion around alternate ways of doing things ('this doesn't block merge, but...') is accepted and expected (without becoming hostile/nit-picky or devolving into bikeshedding -- if it's not blocking, the submitter can always just merge) - Have brown bags to talk about technology in freer, non-deadline-constrained setting (sparks ideas, gets people building tech communication skills) Really interested to hear what thoughts others have on this.

derefr 8y ago
> treating PRs as communication I kind of wish that there was a type of thing like a PR, but marked in a way where you couldn't actually merge it. It'd still get built by CI, if such a thing was configured; but the point of the submission of such a "proposal with prototype" object would be to discuss whether the design represented by the prototype implementation is a design worth going with. The actions applicable to such objects would be "accept and close" or "reject and close." You'd be able to have several of these objects that live under a given issue (i.e. several potential solution-designs to the same problem), and—as long as the issue has at least one proposal-with-prototype object under it—the issue would be in a "pending" state until one such proposal object was Approved, and Approving one proposal object would Reject the others. The point of this would be to replicate the thing that people go through with design discussions on mailing lists when they send in code samples to explain their designs—but with those code samples being working, buildable code, such that the properties of the design proposal can be tested against the current implementation and against any alternative proposals. You could call these objects "RFCs" :)
- ChrisSD 8y ago
  I was just about to link to something like this[0] before I read your last sentence. [0]: https://github.com/rust-lang/rfcs/pull/911 https://github.com/rust-lang/rfcs/pull/911
- nine_k 8y ago
  Hmm. I suppose your CI and code review tools allow code on branches different than master? So you can directly use them to show a what-if PR. I did; it worked, that is, sparked a discussion and led us to designing things in a better way.
  - derefr 8y ago
    Yeah, I was picturing GitLab's CI/CD workflow here, where the RFC objects would get Review Apps built from them. My objection to using PRs for this is that people think the point of a PR is to merge the code, and people tend to nitpick the code during code-review with the goal of making it clean enough to merge. The idea of a separate RFC object is that, unlike a PR, you literally cannot merge an RFC, so there's no temptation to nitpick, or really to talk about anything other than the design. It much more closely mimics the social mores of a mailing-list thread discussing a code snippet. Also, being able to explicitly track Approved and Rejected RFCs on a system level would be nice, to know what discussion needs to be referenced when doing the final implementation. If you just used "what-if" PRs, both the chosen and not-chosen designs' PRs would just end up in the Closed state, and would show up equally in search. Ideally, Rejected RFCs would be filtered out of search by default.
    - jl-gitlab 8y ago
      Interesting idea. We already sort of have a special-case MR (WIP) where merge is disabled until the work-in-progress status is removed. It could be interesting to have an RFC equivalent where no merge is clearly ever intended, but you still get the conversation flow, review app, and so on.
      - Serow225 8y ago
        Definitely might be useful for teams/projects organized enough to have an RFC process.
- organsnyder 8y ago
  My previous team had a convention of putting a prefix "[DO NOT MERGE]" on the subject line of RFC-style PRs. Worked well enough, though it would have been nice to have the tool enforce that also (even a checkbox like "prohibit merging" would have worked).
  - nicoburns 8y ago
    Gitlab actually enforces this for PRs with [WIP] in the title
    - sdoering 8y ago
      Thank you for that tidbit of knowledge. Learned something tonight.
    - dvlsg 8y ago
      There's a browser extension that does something similar for github too. At least for Chrome, anyways. I imagine it works for other browsers though.
  - JohnBooty 8y ago
    Absolutely, this. We did the same thing -- [DONT MERGE] or [WIP] in the title. That way you could get feedback from other devs, and pushing commits to your PR would trigger CI runs so you could be made aware of any build failures you were causing. (The builds took hours, sometimes, so running the full test suite locally was impractical) We used Github but I'm sure it would work in a lot of workflows.
- rakoo 8y ago
  We sometimes have this use case at work where we want to discuss a potential idea and get feedback without merging anything. What we do is open a PR and decline it directly (I'm using bitbucket lingo). This way it can't be merged, and the discussion diverges from the particulars of the diff and focuses more on the idea and the architecture of the change
- kickopotomus 8y ago
  This is pretty conducive to a Gerrit (https://www.gerritcodereview.com https://www.gerritcodereview.com) workflow. If unfamiliar, Gerrit essentially treats each commit as it's own PR or "change" in Gerrit terms. It uses git's refspec as staging area so each commit basically becomes a mini branch. Changing a "change" then requires amending the commit and pushing a new "patch set". So what my team does when we want to prototype something is we create a change and then push separate patch sets for different solutions (there is a nice diff tool to view differences between patch sets). The CI runs for each patch set and we then discuss the merits of each approach and "abandon" (reject) the change and use it for reference going forward.
Fellshard 8y ago
I've been wondering if it would be useful to have tools that help build better narratives for proposed changes. Which files should be viewed in which order? How do the changes tie together? It's not quite literate programming - the commentary would have to be interwoven with the code, but it's not permanently attached to it. Existing comment systems usually approach the interwoven change and commentary alright by interleaving comments into the diff, but they still don't allow you to choose which files or diff chunks are displayed in which order.
- JohnBooty 8y ago
  I'd never thought of that. That's brilliant. I agree: it'd be nice to somehow control the order in which the diffs were presented to the reviewers! This would IMO be a killer feature for Github or Gitlab to implement. Typically, I explain complex PRs "manually" in the PR itself or by screen sharing them with the other devs, but this is not always very efficient.
- btasovac 8y ago
  Thanks for sharing this idea! We have a similar issue regarding this [1], so please feel free to engage in the discussion there and bring some attention to it. We'd love to hear more from you on this matter! [1] - https://gitlab.com/gitlab-org/gitlab-ce/issues/18037 https://gitlab.com/gitlab-org/gitlab-ce/issues/18037
lifeisstillgood 8y ago
I want pre-PRs. Call it email lists, or waterfall planning, or RFCs - but I am fed up starting on tickets / work and finding out that there are 5 different POVs. I think, in short, every business would win a lot from having a PEP-like process where business owners and coders discuss what is wanted. So while I am wishing It shall be illegal, punishable by a week in the stocks to - ask for an estimate verbally for any job that has not had at least 200 words describing the requirements and been responded to with interrogating questions and explanations - to work on any project that does not have a 2 page summary and been broken down into a minimum of 20 seperate 100 word requirements - and a pony
JohnBooty 8y ago
Building a culture where discussion around alternate ways of doing things ('this doesn't block merge, but...') is accepted and expected (without becoming hostile/nit-picky or devolving into bikeshedding -- if it's not blocking, the submitter can always just merge) Yeah, this is IMO an important thing you want to do in code reviews. Specifically, when it's part of an ongoing collaboration and the feedback can be put to use in subsequent reviews. We had soooo much blocking nitpicking and bikeshedding at my last job. The "code artistes" among us would block PRs for these sorts of debatable style issues and other nonessential issues that weren't even remotely blockers IMO. Those discussions were a real sap on productivity and team cohesion. And management was unwilling to give any direction. (It was a particularly big problem on our team because our test suite was a real pig, and moving code through the build/test servers and out to production could take hours sometimes -- so highly debatable nitpicks could result in literal days of lost time) When I wanted to give that sort of non-blocking constructive feedback, I always simply did what you mention: I left the feedback, discussed things with the submitter, and approved the PR. Not rocket science. Although, apparently, it was beyond some of our devs' comprehension.
- mikekchar 8y ago
  One PR does not a code base ruin. Never block a PR unless it's a customer facing issue. If you're getting into a "thin edge of the wedge" situation, or you are having bad actors on your team, address that issue separately. If you need the leverage of blocking a PR to force the conversation, then you have already lost all hope in that team. Find another place to work (for both groups peace of mind). Like you said, some people can't comprehend that. However, it's exactly the same "bad actor" situation. If people are executing a denial of service attack on your process in order to get their own way, then you need to address that situation -- outside the context of the PR. If you can't solve the problem, then it's probably time to consider voting with your feet. Working with bullies is never going to be fun.

by mcfunk 8y ago

derefr 8y ago

> treating PRs as communication I kind of wish that there was a type of thing like a PR, but marked in a way where you couldn't actually merge it. It'd still get built by CI, if such a thing was configured; but the point of the submission of such a "proposal with prototype" object would be to discuss whether the design represented by the prototype implementation is a design worth going with. The actions applicable to such objects would be "accept and close" or "reject and close." You'd be able to have several of these objects that live under a given issue (i.e. several potential solution-designs to the same problem), and—as long as the issue has at least one proposal-with-prototype object under it—the issue would be in a "pending" state until one such proposal object was Approved, and Approving one proposal object would Reject the others. The point of this would be to replicate the thing that people go through with design discussions on mailing lists when they send in code samples to explain their designs—but with those code samples being working, buildable code, such that the properties of the design proposal can be tested against the current implementation and against any alternative proposals. You could call these objects "RFCs" :)

ChrisSD 8y ago
I was just about to link to something like this[0] before I read your last sentence. [0]: https://github.com/rust-lang/rfcs/pull/911 https://github.com/rust-lang/rfcs/pull/911
nine_k 8y ago
Hmm. I suppose your CI and code review tools allow code on branches different than master? So you can directly use them to show a what-if PR. I did; it worked, that is, sparked a discussion and led us to designing things in a better way.
- derefr 8y ago
  Yeah, I was picturing GitLab's CI/CD workflow here, where the RFC objects would get Review Apps built from them. My objection to using PRs for this is that people think the point of a PR is to merge the code, and people tend to nitpick the code during code-review with the goal of making it clean enough to merge. The idea of a separate RFC object is that, unlike a PR, you literally cannot merge an RFC, so there's no temptation to nitpick, or really to talk about anything other than the design. It much more closely mimics the social mores of a mailing-list thread discussing a code snippet. Also, being able to explicitly track Approved and Rejected RFCs on a system level would be nice, to know what discussion needs to be referenced when doing the final implementation. If you just used "what-if" PRs, both the chosen and not-chosen designs' PRs would just end up in the Closed state, and would show up equally in search. Ideally, Rejected RFCs would be filtered out of search by default.
  - jl-gitlab 8y ago
    Interesting idea. We already sort of have a special-case MR (WIP) where merge is disabled until the work-in-progress status is removed. It could be interesting to have an RFC equivalent where no merge is clearly ever intended, but you still get the conversation flow, review app, and so on.
    - Serow225 8y ago
      Definitely might be useful for teams/projects organized enough to have an RFC process.
organsnyder 8y ago
My previous team had a convention of putting a prefix "[DO NOT MERGE]" on the subject line of RFC-style PRs. Worked well enough, though it would have been nice to have the tool enforce that also (even a checkbox like "prohibit merging" would have worked).
- nicoburns 8y ago
  Gitlab actually enforces this for PRs with [WIP] in the title
  - sdoering 8y ago
    Thank you for that tidbit of knowledge. Learned something tonight.
  - dvlsg 8y ago
    There's a browser extension that does something similar for github too. At least for Chrome, anyways. I imagine it works for other browsers though.
- JohnBooty 8y ago
  Absolutely, this. We did the same thing -- [DONT MERGE] or [WIP] in the title. That way you could get feedback from other devs, and pushing commits to your PR would trigger CI runs so you could be made aware of any build failures you were causing. (The builds took hours, sometimes, so running the full test suite locally was impractical) We used Github but I'm sure it would work in a lot of workflows.
rakoo 8y ago
We sometimes have this use case at work where we want to discuss a potential idea and get feedback without merging anything. What we do is open a PR and decline it directly (I'm using bitbucket lingo). This way it can't be merged, and the discussion diverges from the particulars of the diff and focuses more on the idea and the architecture of the change
kickopotomus 8y ago
This is pretty conducive to a Gerrit (https://www.gerritcodereview.com https://www.gerritcodereview.com) workflow. If unfamiliar, Gerrit essentially treats each commit as it's own PR or "change" in Gerrit terms. It uses git's refspec as staging area so each commit basically becomes a mini branch. Changing a "change" then requires amending the commit and pushing a new "patch set". So what my team does when we want to prototype something is we create a change and then push separate patch sets for different solutions (there is a nice diff tool to view differences between patch sets). The CI runs for each patch set and we then discuss the merits of each approach and "abandon" (reject) the change and use it for reference going forward.

Fellshard 8y ago

I've been wondering if it would be useful to have tools that help build better narratives for proposed changes. Which files should be viewed in which order? How do the changes tie together? It's not quite literate programming - the commentary would have to be interwoven with the code, but it's not permanently attached to it. Existing comment systems usually approach the interwoven change and commentary alright by interleaving comments into the diff, but they still don't allow you to choose which files or diff chunks are displayed in which order.

JohnBooty 8y ago
I'd never thought of that. That's brilliant. I agree: it'd be nice to somehow control the order in which the diffs were presented to the reviewers! This would IMO be a killer feature for Github or Gitlab to implement. Typically, I explain complex PRs "manually" in the PR itself or by screen sharing them with the other devs, but this is not always very efficient.
btasovac 8y ago
Thanks for sharing this idea! We have a similar issue regarding this [1], so please feel free to engage in the discussion there and bring some attention to it. We'd love to hear more from you on this matter! [1] - https://gitlab.com/gitlab-org/gitlab-ce/issues/18037 https://gitlab.com/gitlab-org/gitlab-ce/issues/18037

lifeisstillgood 8y ago

I want pre-PRs. Call it email lists, or waterfall planning, or RFCs - but I am fed up starting on tickets / work and finding out that there are 5 different POVs. I think, in short, every business would win a lot from having a PEP-like process where business owners and coders discuss what is wanted. So while I am wishing It shall be illegal, punishable by a week in the stocks to - ask for an estimate verbally for any job that has not had at least 200 words describing the requirements and been responded to with interrogating questions and explanations - to work on any project that does not have a 2 page summary and been broken down into a minimum of 20 seperate 100 word requirements - and a pony

JohnBooty 8y ago

Building a culture where discussion around alternate ways of doing things ('this doesn't block merge, but...') is accepted and expected (without becoming hostile/nit-picky or devolving into bikeshedding -- if it's not blocking, the submitter can always just merge) Yeah, this is IMO an important thing you want to do in code reviews. Specifically, when it's part of an ongoing collaboration and the feedback can be put to use in subsequent reviews. We had soooo much blocking nitpicking and bikeshedding at my last job. The "code artistes" among us would block PRs for these sorts of debatable style issues and other nonessential issues that weren't even remotely blockers IMO. Those discussions were a real sap on productivity and team cohesion. And management was unwilling to give any direction. (It was a particularly big problem on our team because our test suite was a real pig, and moving code through the build/test servers and out to production could take hours sometimes -- so highly debatable nitpicks could result in literal days of lost time) When I wanted to give that sort of non-blocking constructive feedback, I always simply did what you mention: I left the feedback, discussed things with the submitter, and approved the PR. Not rocket science. Although, apparently, it was beyond some of our devs' comprehension.

mikekchar 8y ago
One PR does not a code base ruin. Never block a PR unless it's a customer facing issue. If you're getting into a "thin edge of the wedge" situation, or you are having bad actors on your team, address that issue separately. If you need the leverage of blocking a PR to force the conversation, then you have already lost all hope in that team. Find another place to work (for both groups peace of mind). Like you said, some people can't comprehend that. However, it's exactly the same "bad actor" situation. If people are executing a denial of service attack on your process in order to get their own way, then you need to address that situation -- outside the context of the PR. If you can't solve the problem, then it's probably time to consider voting with your feet. Working with bullies is never going to be fun.