-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
* Add gsoc blog post * Incorporate review suggestions
- Loading branch information
Showing
1 changed file
with
132 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,132 @@ | ||
| --- | ||
| title: "Personal Experiences at Babel #1 — A PR with Unusually High Number of Reviews" | ||
| author: "Peeyush Kushwaha" | ||
| slug: personal-experiences-at-babel-1-a-pr-with-unusually-high-number-of-reviews | ||
| --- | ||
|
|
||
| We landed the [parser support](https://github.com/babel/babylon/pull/587) for | ||
| [the stage-2 decorators spec](https://tc39.github.io/proposal-decorators/) last | ||
| week at Babylon — the parser for Babel. If you don’t know what a decorator is, | ||
| the gist of it is that a decorator gives some concise syntax to affect the | ||
| definition of a class or a class method which you decorate. | ||
|
|
||
| ```js | ||
| @frozen class Foo { | ||
| @configurable(false) | ||
| @enumerable(true) | ||
| method() {} | ||
|
|
||
| @throttle(500) | ||
| expensiveMethod() {} | ||
| } | ||
| ``` | ||
|
|
||
| One of the more remarkable things about this PR was the number of reviews it | ||
| received | ||
|
|
||
| [](https://twitter.com/left_pad/status/877894712476258305) | ||
|
|
||
| Perhaps this could be because decorators really are one of the much hyped about | ||
| features in JavaScript. Angular even considered making their own JS flavor | ||
| called AtScript before they decided to go with TypeScript since they love | ||
| decorators (or as they liked to call it “annotations”) so much. | ||
|
|
||
| Well, there is more to the story. As I was recently discussing with a mentor, | ||
| reviewing PRs is a tough job. Reviewing PRs is comparably as hard as solving the | ||
| bug in the first place was. Apart from the technical aspect of reviewing — which | ||
| is ensuring that the bug is being fixed optimally (by perhaps even getting an | ||
| idea of how they would solve the issue and seeing how the submitted patch | ||
| compares to their idea) — there’s another big hindrance. A reviewer has to be | ||
| aware of the whole issue, the discussion surrounding it, and have familiarity | ||
| with the part of the codebase that the PR makes changes to.<sup><a href="#footnotes">1</a></sup> | ||
|
|
||
| When I initially joined Babel, and was not-so-familiar with the codebase, every | ||
| issue I encountered was almost instantly answered after I posted it in Babel’s | ||
| chatroom, which left me with the (wrong) impression that perhaps the maintainers | ||
| are some god-like figures who know it all and that everyone’s expected to adhere | ||
| to the same fictitious standards. | ||
|
|
||
| Even after becoming familiar with the codebase, I was submitting PRs without | ||
| proper documentation under the assumption that it took me a while to solve the | ||
| issue and see all things, but if the reviewers see the code they’d instantly be | ||
| able to evaluate it just like they were answering my questions. | ||
|
|
||
| Eh! Very wrong. Let me just bust this myth (assuming I’m not the only one who | ||
| has felt it). Even they (maintainers) won’t have all the answers at times, and | ||
| sometimes you’ll have to search for yourself — and that’s how it should be.<sup><a href="#footnotes">2</a></sup> | ||
|
|
||
| In open source, there are a lot of people who want to contribute, but are unable | ||
| to because they don’t know how to code / they don’t know how to present PRs / | ||
| they don’t know what the project wants / they don’t know what the maintainers | ||
| want / a ton of other things. A lot of times you’ll find help along the way, | ||
| but a lot of that is controlled by factors beyond your control.<sup><a href="#footnotes">3</a></sup> | ||
|
|
||
| One of the joys of getting your PR merged is not just the programming but | ||
| somehow making the project move forward in the way it is expected to be moved | ||
| forward. And doing that by somehow identifying what the project needs and being | ||
| able to deliver that. | ||
|
|
||
| In order to merge this PR I had to find people and talk to them — people who use | ||
| decorators, people who are interested in seeing an implementation of decorators, | ||
| people who want to contribute to babel for decorators. After getting consensus | ||
| on how to move forward<sup><a href="#footnotes">4</a></sup>, I had to go through the spec and all the existing | ||
| discussions surrounding it so that my understanding of the spec could be up to | ||
| speed with other people. | ||
|
|
||
| And finally — the most important part which I think got this PR the number of | ||
| reviews that we saw — making it easy for people who’ll be reviewing my PR by | ||
| explaining everything they would need to get up to speed with the whole | ||
| situation. By chance, at the time the PR I made was able to satisfy some of the | ||
| criteria I mentioned earlier: | ||
|
|
||
| 1. Making sure reviewers are aware of the whole issue (by mentioning in detail the | ||
| decisions I’ve taken so they don’t necessarily have to look at the code to | ||
| figure it out) | ||
| 2. The discussion surrounding it (by mentioning alternate viewpoints on some of the | ||
| decisions so as to make it easier to compare them with the decisions made) | ||
| 3. Explaining clearly my strategy to solve the problem (to assist the technical | ||
| aspect of reviewing — so that the reviewers can know what I’ve done and then see | ||
| the code rather than the other way around) | ||
|
|
||
| And that’s what did it! (or so I think). So there’s the mystery unraveled — A PR | ||
| with unusually high number of reviews¹. | ||
|
|
||
| **PS**: I wanted to share my personal experience with this blog post, | ||
| not write a guide to be followed or a technical blog post. Therefore, some | ||
| statements which I make may not hold true in general or may be debatable, so | ||
| they should just be read in the context of the experience I narrate. | ||
|
|
||
| Also note that if you’re looking for decorators support in Babel, we still have | ||
| a long way to go. This is just the parser and work on the transform (which | ||
| converts your code to functionally equivalent ES5) is yet to be done. But now | ||
| that we’ve made the decisions that needed to be made, things will move more | ||
| smoothly from here onwards. | ||
|
|
||
| ## Footnotes | ||
|
|
||
| 1. We have a shortage of manpower when it comes to reviewing PRs. It was also | ||
| recently discussed in one of our weekly meetings ([link to the meeting | ||
| notes](https://github.com/babel/notes/blob/master/2017-06/june-21.md#priority-topics)). | ||
| Perhaps you could help us with this. Drop by [our slack | ||
| chatroom](https://slack.babeljs.io/) and offer your help! | ||
| 2. I feel that the myth stems from the fact that when you’re new to the project the | ||
| mentors definitely do know more about the project than you | ||
| 3. (to illustrate) Some random factors which might affect the chances of you | ||
| getting help: | ||
| - If someone was online who worked on the same thing when you post a question on | ||
| the chatroom | ||
| - Someone who knows your doubt will take a lot of time to deal with and they | ||
| want to give you personal attention and not just throw information at you | ||
| - Someone who’s able to gauge where you’re coming from, and so on. | ||
| 4. We’d been stuck for [a while](https://github.com/babel/babel/issues/2645) since | ||
| *a lot* of people use a nonstandard implementation for decorators which came at | ||
| around the time the spec was in stage-0. The changes in the spec are not | ||
| backwards-compatible so we were unsure on how we should introduce support for | ||
| the new spec without causing much disruption for people who use Babel. We | ||
| finally decided that we’ll be introducing this PR as an opt-in to allow people to | ||
| migrate at their own pace. | ||
|
|
||
| ## Outro | ||
| Peeyush Kushwaha is a [GSoC 2017](https://summerofcode.withgoogle.com/organizations/5842528113786880/#6632162202746880) student. Give him a follow on Twitter: [@PeeyFTW](https://twitter.com/PeeyFTW)! | ||
|
|
||
| This was originally posted [here on medium](https://medium.com/@peey/personal-experiences-at-babel-1-a-pr-with-unusually-high-number-of-reviews-5cb49ee71897). Please check out our first post on [Summer of Code](https://babeljs.io/blog/2017/08/09/babel-and-summer-of-code) for more info! |