SE-EDU
  • Home
  • About
  • Guides for SE student projects »

    GitHub conventions

    Follow the principle of local consistency

    Some of the conventions given below have multiple options. In those cases, or if in doubt, aim to be consistent locally e.g., when naming PRs, follow the convention most other PRs (especially, those done by senior developers) in the repo seem to follow.

    PRs

    PR name

    Situation 1: The PR fully fixes an existing issue in the issue tracker (i.e., the issue can be closed when the PR is merged):

    • Option 1: Just copy-paste the issue title (including issue number) as the PR title.
      Format: IssueTitle #IssueNumber
      e.g. Error alert email has very long subject #5958
    • Option 2: Copy-paste the issue title as the PR title, but tweak into the following format.
      Format: [#IssueNumber] IssueTitle
      e.g. [#5958] Error alert email has very long subject
    • Option 3: Name the PR based on what it does. This is suitable when the issue titles are not expected to be good enough to be reused as PR titles.
      e.g. Shorten the error alert email subject

    Rationale: Duplicating issue title in PR title is for easy tracing between PRs and issues, to compensate for GitHub’s lack of strong linking between the two. Assume there is an invisible prefix in front of the PR title Fix issue: …​

    Situation 2: All other cases (i.e., the issue is only a partial fix to an existing issue or it does not correspond to an existing issue):
    Name the PR as if it is the subject line of a commit that contains the entire PR code.

    Every commit must have a well-written commit message subject line.

    • Try to limit the subject line to 50 characters (hard limit: 72 chars)

    Rationale: Some tools show only a limited number of characters from the commit message.

    Use the imperative mood in the subject line.

    • Good: Add README.md
    • Bad: Added README.md
    • Bad: Adding README.md

    Capitalize the first letter of the subject line.

    • Good: Move index.html file to root
    • Bad: move index.html file to root

    Do not end the subject line with a period.

    • Good: Update sample data
    • Bad: Update sample data.

    You can use scope: change format (when applicable).

    • e.g. Person class: remove static imports
      Unit tests: remove blank lines

    PR description

    • Follow when you submit a PR, GitHub will present you with some boilerplate content to tell you what details to includethe PR template (example), if any.

    • Use the Fixes keyword annotations in the PR description so that the related issue can auto-closed when the PR is merged. e.g., Fixes #1234
      If the PR is a partial fix, use Fixes part of #1234.

    • Give before-and-after screenshots if applicable. If yor change results in UI changes that are not readily apparent from the code, give before and after screenshots to help the reviewer.

    • Give the proposed merge commit message if applicable. If the PR has more than one commit and the PR is non-trivial, propose a commit message for the merge commit.

    Commit messages for non-trivial commits should have a body giving details of the commit.

    • Separate subject from body with a blank line.
    • Wrap the body at 72 characters.
    • Use blank lines to separate paragraphs.

    Example: A commit message for a commit that is part of a multi-commit PR:

    Unify variations of toSet() methods

    There are several methods that convert a collection to a set. In some
    cases the conversion is in-lined as a code block in another method.

    Unifying all those duplicated code improves the code quality.

    As a step towards such unification, let's extract those duplicated code
    blocks into separate methods in their respective classes. Doing so will
    make the subsequent unification easier.

    Tip for SourceTree users: To help you keep the commit message width to a 72 characters:

    1. Choose Tools -> Options.
    2. Click on the General tab, scroll down to the Commit settings section.
    3. Tick the Display a column guide at commit message at [72] characters option.

    Use bullet points as necessary. Instead of relying entirely on paragraphs of text, use other constructs such as bullet lists when it helps.

    Example: A commit message for a bug fix:

    Find command: make matching case insensitive

    Find command is case sensitive.

    A case insensitive find is more user-friendly because users cannot be
    expected to remember the exact case of the keywords.

    Let's,
    * update the search algorithm to use case-insensitive matching
    * add a script to migrate stress tests to the new format

    Explain WHAT, WHY, not HOW.

    • Use the body to explain WHAT the commit is about and WHY it was done that way. The reader can refer to the diff to understand HOW the change was done.

    • Give an explanation for the change(s) that is detailed enough so that the reader can judge if it is a good thing to do, without reading the actual diff to determine how well the code does what the explanation promises to do.
      If your description starts to get too long, that's a sign that you probably need to split up your commit to finer grained pieces. [adapted from: git project]

    • Minimize repeating information that are given in code comments of the same commit.

    Structure the body as follows:

    {current situation} -- use present tense

    {why it needs to change}

    {what is being done about it} -- use imperative mood

    {why it is done that way}

    {any other relevant info}
    • Avoid terms such as 'currently', 'originally' when describing the current situation. They are implied.
    • The word Let's can be used to indicate the beginning of the section that describes the change done in the commit.

    Example: A commit message for a code quality refactoring:

    Person attributes classes: extract a parent class PersonAttribute

    Person attribute classes (e.g. Name, Address, Age etc.) have some common
    behaviors (e.g. isValid()).

    The common behaviors across person attribute classes cause code duplication.

    Extracting the common behavior into a super class allows us to use
    polymorphism when dealing with person attributes. For example, validity
    checking can be done for all attributes of a person in one loop.

    Let's pull up behaviors common to all person attribute classes into a new
    parent class named PersonAttribute.

    Using inheritance is preferable over composition in this situation
    because the common behaviors are not composable.

    Refer to this S/O discussion on dealing with attributes
    http://stackoverflow.com/some/question

    Refer to the article How to Write a Git Commit Message for more advice on writing good commit messages.

    Example:

    Fixes #1234

    [Details of the PR...]

    Before:
      [screenshot]

    After:
      [screenshot]

    Proposed commit message:

    commit message goes here

    more ...

    PR merge commit

    When merging a PR branch to the main branch, use one of these formats for the subject line of the merge commit.

    • Option 1: Issue Title #IssueNumber (#PrNumber)
      e.g. Error alert email has very long subject #5958 (#6580)

    • Option 2: [#IssueNumber] Issue Title (#PrNumber)
      e.g. [#5958] Error alert email has very long subject (#6580)

    • Option 3: PR Title (#PrNumber) (i.e., based on PR only, issue info omitted)
      e.g. Error alert email has very long subject (#6580)

    Pick one option and use it consistently in the entire code base.

    Rationale: This format allows easy traceability among a merge commit, the issue it fixes, and the PR that fixed it. Having the issue name tells us what the commit is about without having to look it up in GitHub issue tracker.

    Issues

    • Issue title should be concise yet descriptive. For example, instead of Newbie question, please help, use How do I set up git to ignore test files?

    • Follow when you submit an issue, GitHub will present you with some boilerplate content to tell you what details to includethe issue template (example), if any.