-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add the gsoc blog post #1279
Add the gsoc blog post #1279
Changes from 1 commit
822e4f3
1d92ef6
2935b4b
0c75299
cc04f11
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,129 @@ | ||
--- | ||
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 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 the times you’ll find help along the way, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
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 find people and talk to them — people who use | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
also, babel -> Babel
|
||
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 | ||
discussion 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 experience with this blog post, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Nit: extra experience |
||
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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Missing space between |
||
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 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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
mentors definitely do know more about the project than you so | ||
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 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 a opt-in to allow people to | ||
migrate at their own pace. | ||
5. Originally posted [here on medium](https://medium.com/@peey/personal-experiences-at-babel-1-a-pr-with-unusually-high-number-of-reviews-5cb49ee71897) | ||
|
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.