On this page

Contributing

Coding Rules

To ensure consistency throughout the source code, keep these rules in mind as you are working:

  • All features or bug fixes must be tested by one or more specs (unit-tests).
  • An automated formatter is available
  • A Pull Request (PR) should be used for merging your code to the general codebase.

Development Workflow

Instead of one (master) we have two (master, develop) core branches. master will contain the stable verions (tagged with a version number i.e. v1.0.0), while develop will be the working branch.

New Features

Branch is ALWAYS created from develop, and prefixed by feature/

  1. Issue assigned to developer
  2. Issue is marked as "In Progress" and broken down to actionable tasks (either as a task list in the original issue, or as different sub-issues)
  3. As they are completed, tasks/sub-issues are checked/closed so the progress can be monitored
  4. When feature is completed, a PR is created and assigned to a different developer, who will provide a review (see Review Report on how to perform a constructive code review). The issue is now marked as "Under Review". If changes are required, then the issue is reverted back to "In Progress".
  5. When a PR is approved, it is merged into the develop branch by the developer and the issue is automatically closed (using appropriate issue linking in the PR)
  6. When a PR is merged into develop, the CI will automatically build a new docker image and will deploy the feature to http://develop.s5labs.eu to be tested by the developer. If everything is OK, then no additional steps are needed. Otherwise, the issue should be re-opened and steps 2-6 are repeated.

PR Reviewers

  • Apollo Services:

    • Harvester: Evangelos, Konstantinos
    • Mapping: Konstantinos, Sotia
    • Cleaning: Konstantinos, Sotia
    • Anonymisation: Dimitris, Evangelos
    • Encryption: Evangelos, Konstantinos
    • Loader: Konstantinos, Sotia

  • Griffo Services:

    • Data Preparation: Alexandros, Dimitris B.
    • ML: Andreas, Erifili, Minas
  • Backend & Frontend: Depends who created the initial (or related) implementation. Ask in Slack
  • Search & Predictor: Ask in Slack

Releases

  • We schedule releases, where multiple features are grouped together, reviewed by Marios (Frontend/Backend), Valanto (Frontend), George (Apollo/Griffo Services), Evmorfia (Griffo Services) and merged into master, where they get a new "feature" version (i.e. 2.x.0).
  • CHANGELOG is generated automatically (make sure that the commit messages of the PRs follow the Confentional Commits specification)
  • The release then is MANUALLY deployed to https://demo.s5labs.eu, to be tested by the QA team.
  • Any bugs found in demo, are opened as different issues (no issue re-opening for features) and handled as described below.

Bug Fixes

https://develop.s5labs.eu

Fixes are applied directly by repeating steps 2-6 above.

https://demo.s5labs.eu

Branch is ALWAYS created from master, and prefixed by fix/

The process here has the following differences:

  1. Review is performed by:

    • Frontend: Marios, Valanto
    • Backend: Marios
    • Apollo Services: George
    • Griffo Services: Evmorfia, George
  2. PR is merged by the reviewer into both master and develop branches
  3. CI will generate the appropriate versions (patch version -i.e. 2.0.x in demo- and latest for develop)
  4. No scheduled releases. Deployment is performed as soon as possible/necessary

Customisations

Branch is created from master, unless a customisation/XXX branch exist, in which case we use that

  1. Review is performed by the person responsible for the customisation
  2. PR is merged by the reviewer to the appropriate branch

Pull Requests

  • Every pull request should try to resolve a single issue to make sure it is easy to review. Try to keep the PRs small but complete. This makes it a lot easier to review and keep track of progress
  • It should also have screenshots of the feature (if it involves frontend)
  • Squash and Merge after it’s been accepted. Also edit the (PR) commit message so that it is descriptive of the feature. This helps keeping the commit log clean, since every commit on master is a working copy of the product (see: Commit Message Guidelines). If anything breaks, going back a commit should be very simple
  • While you are working on the PR, keep the [WIP] tag in the title.
  • In the description add a small list of every thing you are working on, and check off boxes as you finish them. This helps reviewers track your progress on the PR so they know how close to done you are. If the PR is related to an issue, add a closing reference to an issue as well.

Code Reviews

Every PR should be reviewed by the component dev lead and/or one other team member. You learn a lot of how to write better code by reviewing other people’s, even if you don’t have a lot of experience. Reviewing code is a great way to catch bugs early and avoid growing technical dept.

Guidelines - Process

  1. Multiple Passes
  2. Code style will be generally enforced through ESLint & Prettier for Javascript/Typescript and Black for Python

  3. Does it work?

    • You will be surprised how not uncommon it is that people submit broken or nonfunctional code
    • Are there any bugs?

  4. High level feature architecture/design

    • If you had to pseudocode the feature at a very high level with a bunch of blackboxes to cover specific implementation details, does the developer’s code follow that structure? Or are they going about it the wrong way?
    • The developer might be only thinking about their feature and not the rest of the app or how well it integrates. That’s fine initially since they aren’t expected to know everything, but you should point out anything that won’t integrate or scale well. Ideally these things should be communicated before the dev starts designing and coding.

  5. Code quality

    • Code structure
    • Functions/components broken down enough but not too much
    • Is it DRY?

    • Is the code safe?

      • Does it make the necessary checks?
      • Is it prone to bugs in the future?

    • Mainly, is the code clear to you? Is it maintainable?

      • Are variables clearly named?
      • Is the purpose of each chunk of code reasonably obvious?
      • Is anything ambiguous clarified with a comment?
      • Are edge cases and valid values for parameters outlined in comments and checked-for in code?
      • Are constants being used when needed?
  6. Formatting / Cleanliness / Best Practices / Consistency

Comments / Review Report

Remember to use positive criticism and be careful about your wording:

  • Suggest instead of command
  • Try to provide reasoning on your suggestions

  • Always give positive feedback first:

    • "I like how you..."
    • "Great job on..."

  • Phrases you that are great:

    • "I would do ... instead"
    • "Maybe you could do this ... because ..."
    • "... would be better because ..."
    • "I noticed a couple ..."

  • Phrases that aren’t great:

    • "You should do..."
    • "That is a bad way of doing it"

Being strict on code reviews is a good thing because people will learn more, get a better grasp on what good/bad code is, and will get better with time. However, you also need to be mindful of your project timeline. If time is short and there’s a lot to do, you may need to prioritise shipping functional code than half baked, well written code.

Commit Message Guidelines

We have very precise rules over how our git commit messages can be formatted. This leads to more readable messages that are easy to follow when looking through the project history. But also, we use the git commit messages to generate the change log. Since we use Squash and Merge, and only the commit message of the PR is kept, you only need to follow this rules in your PR commit message.

Format

The commit message of the PR consists of a type, a scope (optional) and a subject, and should not be longer than 100 characters (this allows the message to be easier to read on GitHub as well as various git tools).

Type

Must be one of the following:

  • build: Changes that affect the build system or external dependencies (example scopes: pip, npm)
  • ci: Changes to our CI configuration files and scripts (example scopes: actions, docker)
  • docs: Documentation only changes
  • feature: A new feature
  • fix: A bug fix
  • perf: A code change that improves performance
  • refactor: A code change that neither fixes a bug nor adds a feature
  • style: Changes that do not affect the meaning of the code (white-space, formatting, missing semicolons, etc)
  • test: Adding missing tests or correcting existing tests

Scope

The scope should be the name of the module affected (as perceived by person reading changelog generated from commit messages. For example:

  • backend: Generic backend change
  • frontend: Generic frontend change
  • user: User module
  • auth: Authentication module
  • organisations: Organisations module

There are a couple of exceptions to this rule:

  • packaging: used for changes that change the npm package layout in all of our packages, e.g. public path changes, package.json changes done to all packages, d.ts file/format changes, changes to bundles, etc.
  • changelog: used for updating the release notes in CHANGELOG.md
  • none/empty string: useful for style, test and refactor changes that are done across all packages (e.g. style: add missing semicolons)

Subject

The subject contains succinct description of the change:

  • use the imperative, present tense: "change" not "changed" nor "changes"
  • don’t capitalise first letter
  • no dot (.) at the end

Breaking Changes

Breaking Changes body (description) should start with the word BREAKING CHANGE: with a space or two newlines. The rest of the commit message is then used for this.

Revert Changes

If the commit reverts a previous commit, it should begin with revert:, followed by the header of the reverted commit. In the body it should say: This reverts commit <hash>., where the hash is the SHA of the commit being reverted.