Instead the article is some strange "self-help"/"hustle-hard" blogspam, quoting Malcolm Gladwell and shading a picture of the normal distribution and pretending it's arrived at some brilliant insight.
Instead the article is some strange "self-help"/"hustle-hard" blogspam, quoting Malcolm Gladwell and shading a picture of the normal distribution and pretending it's arrived at some brilliant insight.
- 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.
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" :)
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.
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.
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.
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.
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.
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.
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.
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.
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