Skip to content

UniOffice Developer Guide

Jump to bottom
Gunnsteinn Hall edited this page Jul 28, 2019 · 2 revisions

UniOffice Developer Guide

The goal with this document is to define what conventions to follow when contributing to UniOffice. This helps unify the codebase and makes it easier for new contributors to get started. It can be referenced when reviewing Pull Requests.

It is a work in progress that will evolve and improve over time. If there are any comments, feel to contact us or file an issue.

Overview:

1. Follow recommended best practices

We generally try to follow https://github.com/golang/go/wiki/CodeReviewComments although there are a few exceptions, notably

  • We are flexible on the case of acronyms, for example we prefer "Pdf" in type names rather than "PDF", or "Id" rather than "ID"

2. Use goimports

Run goimports to clean up imports and formatting within the entire project.

3. go test passing at all times

That is:

go test -v github.com/unidoc/unioffice/...

should pass (without exception).

4. go vet returning 0 issues

Go vet should be passing at all times. That is:

go vet github.com/unidoc/unioffice/...

should not return any issues.

5. golint not regressing

Over time we hope to be fully (or at least mostly) compatible with golint, i.e. golint returning as few issues as possible.

Ensure that golint issue count is not increasing on new contributions. I.e.

golint github.com/unidoc/unioffice/... | wc -l

should be equal or lower as the base branch that the PR is made to (typically master).

6. Test coverage

All code should have test coverage, aiming for 90% coverage, although flexible depending on complexity. We try to be practical: No need to test every single getter and setter.

To assess test coverage, run

go test -cover github.com/unidoc/unioffice/...

on both the feature branch and compare with output from master. Ensure that there is no regression. Packages should aim for 90% coverage.

7. License header in each go file

Ensure that the license header is present in the beginning of each go file before the package name. Should start with:

// Copyright 2017 FoxyUtils ehf. All rights reserved.
//
// Use of this source code is governed by the terms of the Affero GNU General
// Public License version 3.0 as published by the Free Software Foundation and
// appearing in the file LICENSE included in the packaging of this file. A
// commercial license can be purchased by contacting sales@baliance.com.

package ...

8. Avoid use of dot imports in new code

Our goal is to eliminate use of dot imports in the code base over time. While dot imports between the model and core package are safe as we fully control both namespaces, it is not a recommended practice and will be refactored out progressively.

This also helps making the interface of the core package simple so that code without dot import can be readable and simple to work with.

9. Prefer type switches for multiple types

Use .(type) switches in preference to multiple type assertions, except if there is only a single type check.

func (m *CoreProperties) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
	m.CT_CoreProperties = *NewCT_CoreProperties()
lCoreProperties:
	for {
		tok, err := d.Token()
		if err != nil {
			return err
		}
		switch el := tok.(type) {
		case xml.StartElement:
			switch el.Name {
			case xml.Name{Space: "http://schemas.openxmlformats.org/package/2006/metadata/core-properties", Local: "category"}:
				m.Category = new(string)
				if err := d.DecodeElement(m.Category, &el); err != nil {
					return err
				}
			case xml.Name{Space: "http://schemas.openxmlformats.org/package/2006/metadata/core-properties", Local: "contentStatus"}:
				m.ContentStatus = new(string)
				if err := d.DecodeElement(m.ContentStatus, &el); err != nil {
					return err
				}
			case xml.Name{Space: "http://purl.org/dc/terms/", Local: "created"}:
				m.Created = new(unioffice.XSDAny)
				if err := d.DecodeElement(m.Created, &el); err != nil {
					return err
				}
		case xml.EndElement:
			break lCoreProperties
}

rather than

func (m *CoreProperties) UnmarshalXML(d *xml.Decoder, start xml.StartElement) error {
	m.CT_CoreProperties = *NewCT_CoreProperties()
lCoreProperties:
	for {
		tok, err := d.Token()
		if err != nil {
			return err
		}
		if el, ok := tok.(xml.StartElement); ok {
			switch el.Name {
			case xml.Name{Space: "http://schemas.openxmlformats.org/package/2006/metadata/core-properties", Local: "category"}:
				m.Category = new(string)
				if err := d.DecodeElement(m.Category, &el); err != nil {
					return err
				}
			case xml.Name{Space: "http://schemas.openxmlformats.org/package/2006/metadata/core-properties", Local: "contentStatus"}:
				m.ContentStatus = new(string)
				if err := d.DecodeElement(m.ContentStatus, &el); err != nil {
					return err
				}
			case xml.Name{Space: "http://purl.org/dc/terms/", Local: "created"}:
				m.Created = new(unioffice.XSDAny)
				if err := d.DecodeElement(m.Created, &el); err != nil {
					return err
				}
		}
		if _, ok := tok.(xml.EndElement); ok {
			break lCoreProperties
		}
}

In general prefer switches to multiple if else as go checks for missed cases.

10. Doc comments

All exported names should have godoc-styled comment, as should any non-trivial unexported names. See https://golang.org/doc/effective_go.html#commentary for more information about commentary conventions.

When referencing a variable in the godoc-styled comment, place the variable name in ticks, example:

// CopySheet copies the existing sheet at index `ind` and puts its copy with the name `copiedSheetName`.
func (wb *Workbook) CopySheet(ind int, copiedSheetName string) (Sheet, error) {

11. Test and supplementary files

Test and supplementary data should be placed in a testdata folder under the relevant package. If used by more than one package, then can be placed in the testdata folder of the more fitting package or a more toplevel testdata folder, depending on what is the most fitting location for the files in question (e.g. what package uses those files the most etc).

12. TODO/FIXME/NOTE comments should refer to author by GitHub handle

For example

// FIXME(gunnsth): Not working for case t = 3.

Also, let's use the tags TODO/FIXME/NOTE and avoid XXX as the meaning is more clear.

13. Error handling - don't panic

We don't use panic under any circumstances. The library should never crash due to a bogus input file.

Errors should generally be passed up the stack

err := doStuff()
if err != nil {
  unioffice.Log("ERROR: doStuff failed: %v", err)
  return err
}

14. Variable declarations

A basic knowledge of type inference as per https://tour.golang.org/basics/14 is assumed (for int, float64).

When declaring and initializing variables, follow the convention of declaring what matters, e.g. make the value and type explicit if it matters.

i) If the initial value matters, make it explicit

i := 0

ii) If the type matters, make it explicit

var offset uint64

iii) If both initial value and type matters, make both explicit (except if obvious: bool, string, int, float64)

offset := uint64(0)
useX := false

Cases where it is obvious (type inference)

pi := 3.14  // float64
i := 0 // int
str := "string of runes" // string
useBorders := false // bool

iv) If the initial value does not matter declare using var.

var i int

v) When declaring multiple variables that are related, enclose within a var block. See also https://golang.org/doc/effective_go.html#commentary

vi) When declaring empty slices, prefer

var t []string

over

t := []string{}

as per https://github.com/golang/go/wiki/CodeReviewComments#declaring-empty-slices

vii) Empty maps When the map size is known a-priori, use

  m := make(map[x]y, 10)

otherwise prefer

  m := make(map[x]y)

over

  m := map[x]y{}

although in terms of performance it is equivalent and only a matter or preference. For consistency most of unidoc code uses the recommended approach.

For maps with pre-defined data, use a map literal:

commits := map[string]int{
    "rsc": 3711,
    "r":   2138,
    "gri": 1908,
    "adg": 912,
}

Examples

  1. Iterator - initial value matters.
for i := 0; i < 20; i++ { ... }
  1. Default value - can change based on logic - initial value matters, type matters, float64 - inferred.
tx := 2.0 // Default offset.
if condition {
    tx += 1.0
}
  1. Variable declaration where initial value does not matter.
var alignment quadding
if condition {
   alignment = quaddingLeft
} else {
  alignment = quaddingRight
}

although this could be better written as

alignment := quaddingRight
if condition {
  alignment = quaddingLeft
}

15. Avoid unnecessary dependencies

Dependencies can be very useful, while they can also be a burden and require updating and testing to ensure non-regression. Thus, we generally prefer to avoid dependencies for any minimal tasks. As a rule of thumb that could mean 500+ lines of unioffice code although that is up for discussion for each case.

16. Import group styles

The style used in imports for consistency is:

import (
    // standard library imports

    // external dependency imports

    // unioffice public imports

    // unioffice internal imports
)

For example:

import (
	"errors"
	"io/ioutil"
	"strings"

	"golang.org/x/text/unicode/norm"

	"github.com/unidoc/unioffice/color"
	"github.com/unidoc/unioffice/common"
	"github.com/unidoc/unioffice/measurement"
	"github.com/unidoc/unioffice/presentation"

	"github.com/unidoc/unioffice/schema/soo/dml"
)

17. Blank lines

Blank lines can be great to separate logic, but they should not be used randomly. Examples:

  1. Blank lines should not appear- at top of a block. Prefer
func x() {
    return
}

// and

func x() {
    if 2 > 1 {
        return
    }
}

rather than (bad)

func x() {

    return
}

// or

func x() {
    if 2 > 1 {

        return
    }
}
  1. Blank lines should not appear at bottom of blocks. Prefer
func negative(x float64) {
    return -x
}

func redundant() {
    if 3 < 4 {
        fmt.Printf("Great\n")
    }
}

over

func negative(x float64) {
    return -x

}

func redundant() {
    if 3 < 4 {
        fmt.Printf("Great\n")

    }
}
  1. Blank lines are great to separate logic within long blocks e.g.
if condition1 {
    // many lines of code ...
}

if condition2 {
    // many lines of code ...
}

18. No naked returns

Named returns are great to indicate the meaning of the return values. Especially if there are more than 2 return values or if the name of the function does not indicate what it returns.

However, using them with a naked return makes the return statement less explicit.

Prefer:

func x() (err error) {
    return err
}

over

func x() (err error) {
    return
}

19. No touching schema package manually

The schema package (github.com/unidoc/unioffice/schema) is autogenerated by a private schema generator so all changes to the schema package should be made by adjusting the schema generator.

Clone this wiki locally