How Square Company Writes Submission Information

Submitting information in Square is an important form of communication, so it is a good way to save time for everyone to write the submission information well. We often hope that we can do better in the past. The submissions were still there, and many were still very important.

We collect ideas that we put into excellent submissions, not only to save our time in the short term, but also in the long run. Now let's share them in the hope that they will also help your team.

TL; DR: Excellent submissions explain changes

In Square, we wrote a lot of code. The best code should be self-evident. When our code is not expressive enough, there will be comments explaining what this code does, or further, why it does so. If a developer reads the SHA code, he can understand what it does and why it does it that way.

Submitting information is not about code, it's about change itself. This is a crucial difference, but it is often overlooked.

When we submit, we cover some code that is ideally clear and self-explanatory with others that are clear and self-explanatory. No matter how clear the code is before and after modification, there will still be a difference in content between the two. Submitting information is a bridge between these differences.

There are two readers submitting information

Consider the reader who submitted the information. There are two general categories: code inspectors and gravediggers.

The direct reader should be the code inspector, who needs to decide whether to accept the change and whether the change reflects the author's intentions. In order to evaluate, code inspectors need to understand the background of the change. For them, it would be much easier if they had information to provide background knowledge for the code change, instead of inferring from the code before and after modification.

In the long run, the reader submitting information should be a developer who tries to fix a problem in the software, remove code that is no longer used, develop new functions based on existing code, or just make similar changes, or several of these things. Sometimes, a developer tries to understand a decision in the past (usually the author himself!). This is archaeology, and if every submission has a helpful message explaining these changes, it becomes very easy. Otherwise, gravediggers have no alternative but to reconstruct the submitter's thinking process by comparing the content before and after the Revision -- a task that is usually impossible to accomplish.

In fact, they have three readers.

Excellent submission of information also helps us to promote our best engineers. They provide some content and details that can help us assess the technical sensitivity of an engineer's daily work. Equally important, excellent submission of information demonstrates our skills and empathy as communicators. Clear writing, insightful and helpful submission of information shows that an engineer is visionary and considerate. Technical competence, empathy and effective communication are all key factors in getting a promotion in Square.

How do you do it?

We are very clear.

Are we fixing a problem? We will briefly describe the expected and actual behavior, and how changes relate to the problem. Are we adding a new feature? We will summarize this function.

It's helpful to assume that the reader knows nothing about why we need to make changes. For most readers, this is true - even the smallest and most "obvious" change. Writing down the motivation for revision is particularly critical.

We're very precise.

This seems self-evident, but in response to review or other iterations, it is easy to ignore updates to submit information. To prevent inaccuracy, submitters and inspectors usually double-check submissions before merging.

We will explain why this modification is safe.

Especially when we modify some basic function code or deal with something unusual, it's worth recording why we did it right. Writing our thoughts and thoughts into the record will give readers some protection in case problems arise later.

Use more background to point to the source

Jira is sure. Design documents, comments in google groups and discussions in Slack are also appropriate. The more background we provide, the easier it will be for gravediggers to understand.

Of course, simple links to a Jira tag or discussion posts are not very useful. Why should readers (who may not be reading the submissions in browsers) go elsewhere to learn about these changes? They may be impatient, so they may lose some key details. They may not be able to view this information at the same time! For example, we used Github Enterprise Edition. Now all of Github's PR information has been lost. Tools have been changing, but submission of information has always existed.

Jira and Stah are very helpful in getting some background information to understand what happens back and forth when deciding to modify. Our submission information captures the final results back and forth, and even if the link is invalid, the submission information should exist independently.

We know the form.

Our information will be read by a variety of tools and scenarios -- pull request, IDEs, git log command lines that may show concise or complete information. These important information is needed about the form of writing:

• 1. Submitting information has a subject line. The first 50 characters on the first line of a submission are treated specially by most tools. In many cases, when reading through git log output, if there is no other operation, you can only see 50 characters. So try to generalize your submission to less than 50 characters. If possible, include JIRA's issue ID.
• 2. Most tools want a blank line between the subject line and the message body.
• 3. Submitted information is often read through command-line tools, usually with branches on the left ("railroad tracks"). Writing by line is very beneficial. Generally, each line is controlled at about 72 characters, and definitely not more than 78 characters (space for "track").

Non-transboundary

We are very cautious in choosing the form for our information.

• Architecture and broad decisions are written into design documents
• Write it in the code comment when you need to explain what the code does and why.
• For code changes, we write them into submissions (you understand!).
• Finally, any code reviewer is concerned, but the gravedigger will not care about the information, written in the pull request comments.

Note: Encourage the above quotations to each other!

The mistakes we made

As you can see from the above, many ideas are included in a good submission and, like most things, need practice and mutual help to build habits. Here are some of our common mistakes.

Blurred gestures

Sometimes we write down information like "Fix < bug >" or "Adds < functionality >". Sometimes this information does not describe the bug or functionality. These are just hints and clues, and the rest of the work needs to be done by the reader.

Interpretation code

Describing new code and interpreting changes are different. Sometimes we help readers understand how this code works by describing the mechanism of the code. However, code comments should be more appropriate for this situation. Sometimes we try to explain high-level design or architecture, which is best placed in the design documents that we can link to. No matter what level of interpretation, there is a better place to write than to submit information.

Translate change into literal meaning

Unfortunately, this is very common. In a hurry, we write simple and straightforward descriptions of changes in information. "Deleted foo" or "Rename bar to baz". That's right, but information like this is too literal. They lack background, motivation, and the security that readers seek.

Example

fault

Here's the really meaningless submission I've written down. Sorry, my friends!

commit ee46355557abe4ebc816f6cef9821e64c25b22f0
Author: Logan Johnson
Date:   Wed Dec 9 17:49:35 2015 -0500

    GET /account/status

diff --git a/common/services/src/main/java/com/squareup/server/account/AccountService.java b/common/services/src/main/java/com/squareup/server/account/AccountService.java
index da02d2f..20c7c43 100644
--- a/common/services/src/main/java/com/squareup/server/account/AccountService.java
+++ b/common/services/src/main/java/com/squareup/server/account/AccountService.java
@@ -48,11 +48,11 @@ public interface AccountService {
    * @deprecated use {@link #status()}
    */
   @Deprecated
-  @POST("/1.0/account/status") //
+  @GET("/1.0/account/status") //
   void status(SquareCallback<AccountStatusResponse> callback);

   /** Queries the account status. */
-  @POST("/1.0/account/status") //
+  @GET("/1.0/account/status") //
   AccountStatusResponse status();

   /** Saves all preferences to server. Any null values will be left unchanged on the server. */

You can see that it doesn't provide any background or motivation for change, it just describes the code. I don't know why I changed the request from POST to GET. Is there a bug? Maybe, but what is a bug? Is it the request itself? I'm fixing it. Our REST Library A bug in it may have been fixed now? Should this change be restored at some point in time? Where are the specific errors in using POST, and why am I better off using GET? So many questions! Now I don't know the answer.

Better

I think I'm doing a better job here:

commit cb5f051c5ee95e795dedaf3a264062cf75d44b1e
Author: Logan Johnson
Date:   Mon Aug 8 14:41:04 2016 -0400

    don't register Transaction in multiple scopes

    RA-14998

    TransactionBundler used to implement both Bundler and PausesAndResumes.
    In order to correct a lifecycle tangle (obvious from the dual
    implementation), we separated those two concerns and made Transaction
    implement PausesAndResumes instead. That change was made in this PR:

    https://stash.corp.squareup.com/projects/ANDROID/repos/register/pull-requests/10689/overview

    Unfortunately, a side effect of (or possibly, motivation for) having the
    TransactionBundler implement PausesAndResumes is that we vend a
    TransactionBundler instance to each caller. That instance would end up
    getting registered with an Activity scope, which was safe since each
    caller would get a new instance-- even though Activity scopes overlap, a
    single PausesAndResumes instance would only ever be registed with a
    single scope.

    By making the Transaction implement PausesAndResumes, we created a
    problem. Transaction is scoped to the Application, and outlives any
    Activities. Since Activity lifecycles (and thus scopes) overlap, this
    meant we would attempt to register the Transaction in an incoming
    Activity scope (via PauseAndResumeRegistrar) while it was still
    registered in the outgoing Activity's scope. This caused a pretty
    popular crash.

    The fix here is to return to vending a PausesAndResumes object to each
    caller, so that a separate instance is registered with each Activity
    scope, avoiding the dual-registration crash.

    note: The attentive reader will see that it's not actually the
    PausesAndResumes that gets registered with the Scope. It's another
    Scoped object that wraps the PausesAndResumes and delegates its
    equals/hashCode.

diff --git a/register/src/main/java/com/squareup/payment/Transaction.java b/register/src/main/java/com/squareup/payment/Transaction.java
index fd09de4..1121c5e 100644

This time, we have more answers. Here are some historical introductions, including links to this issue, for more information. To make the reader understand, I described the bug I fixed in detail (at least I can understand it in eight months). Finally, with these background support, I briefly described what changes I made and why the changes could fix the problem. It's much more useful for me to see this today, and I hope it's also helpful for others.