From 9320f2e6b5dd795e9f4cc2f431e7031b055b3cf6 Mon Sep 17 00:00:00 2001 From: Thomas Juranek Date: Wed, 24 Aug 2022 11:46:36 -0500 Subject: [PATCH] organize coding guidelines into sections and reword items sentance casing on headings --- docs/CodingGuidelines.md | 286 +++++++++++++++++++++------------------ 1 file changed, 151 insertions(+), 135 deletions(-) diff --git a/docs/CodingGuidelines.md b/docs/CodingGuidelines.md index 66003167d..b0e9a5c55 100644 --- a/docs/CodingGuidelines.md +++ b/docs/CodingGuidelines.md @@ -1,201 +1,217 @@ - # Coding guidelines Guidelines that code authors and code reviewers are expected to adhere to in the development of IoT App Kit. -## General guidelines +--- -1. Documentation must be written in a style that is consistent with the rest of the documentation (based on the AWS documentation style) - Compare writing to other written examples within https://docs.aws.amazon.com/iot-sitewise/ as a primary source on how to write the documentation, as well as cross-referencing other documentation found within https://github.com/awslabs/iot-app-kit/tree/main/docs. +## Table of contents -1. Ensure that all documentation is correctly referenced in the table of contents, which can be found at https://github.com/awslabs/iot-app-kit/blob/main/README.md. +- [Components](#components) +- [Code Style](#code-style) +- [Documentation](#documentation) +- [Pull Requests](#pull-requests) +- [Tests](#tests) - In-depth guidelines on documentation: https://alpha-docs-aws.amazon.com/awsstyleguide/latest/styleguide/awsstyleguide.pdf#aws-implementation-guides. This can be a useful reference, but you are not expected to read through and be 100% adherent to these guidelines. -1. Components must have high quality testing, including integration tests which utilize the component in the way a user does. An example of a good example integration test can be found in the [@iot-app-kit/source-iotsitewise package](https://github.com/awslabs/iot-app-kit/blob/main/packages/source-iotsitewise/src/time-series-data/data-source.spec.ts#L562-L603) +--- -1. Tests should be written to maximize the chance that the test would actually catch a real issue, i.e. the likelihood of preventing a regression. This is the primary value of a test. Useful talk on testing: https://www.youtube.com/watch?v=Fha2bVoC8SE3. +## Components -1. Tests should be written to minimize the pain they inflict on the development cycle. Every test represents some amount of burden, through the necessity to run it, maintain it, and alter the test as the codebase morphs, however some tests inflict more pain than others. +- #### General guidelines + - All components prior to release must have public facing documentation within https://github.com/awslabs/iot-app-kit/tree/main/docs, including updating the https://github.com/awslabs/iot-app-kit with the relevant information. - *EXAMPLES OF PAIN*: - - - tests failing when nothing broke - - flaky tests - - snapshot tests that vary on machine - - tests that require refactoring when irrelevant portions of the codebase are altered + - Components must properly handle both loading and error states. -1. Tests should be written to not fail when things are not broken (this helps reduce inflicted pain). This can be achieved through techniques such as not over asserting. + - Components must support both [annotations](https://synchrocharts.com/#/Features/Annotation) and [trend lines](https://synchrocharts.com/#/Features/Trends) where they make sense. - *GOOD ASSERTION EXAMPLE*: - - ```expect(result).toEqual(expect.objectContaining({ error: null })``` + - Components must work across any broswer that has [over 1% of total internet usage](https://caniuse.com/usage-table) - which doesn't include IE11. - *BAD ASSERTION EXAMPLE*: - - every test asserting on every property over and over whether or not they are relevant to the purpose of the test + - Default to [Cloudscape](https://cloudscape.design/) for primitive components unless a good case is made not to. + + - Styling should adhere to [Cloudscape](https://cloudscape.design/) design principles and guidelines. + + - All IoT App Kit components contain a `size` property, which can be "XS", "S", "M", "L", "XL", or "XXL". These sizes do not affect the overall width and height of the element, but instead form a guide for sizing widget elements such as font size, line thickness, symbol size, and level of detail. Each size has a standard sizing for various elements, as well as a dimension which the widget should look good at. For example, an XS widget should be displayable at a size of 75px by 75px, but a user could display a XS widget in a larger container if they desired to. However a XXL widget if put in a small container, will not work well. (Note: this specification is not exhaustive and subject to change. Size-affected elements will vary by component. + + | Size | Primary font size | Large font size |Icon size | Looks good at dimension | + |------|-------------------|-----------------|----------| ------------------------| + | XS | 14 | 20 | 16 | 75px by 75px | + | S | 16 | 24 | 20 | 100px by 100px | + | M | 20 | 32 | 32 | 150px by 150px | + | L | 24 | 48 | 48 | 200px by 200px | + | XL | 32 | 60 | 60 | 300px by 300px | + | XXL | 48 | 96 | 96 | 500px by 500px | + + - Components must support internationalization via messageOverrides (i.e. avoid hard coded phrases). + - Example: + ``` + + ``` + + - Components must have high quality testing, including integration tests which utilize the component in the way a user does. An example of a good example integration test can be found in the [@iot-app-kit/source-iotsitewise package](https://github.com/awslabs/iot-app-kit/blob/main/packages/source-iotsitewise/src/time-series-data/data-source.spec.ts#L562-L603). + +- #### Data + - Components should support various data sources, via either a query or queries property. + - Example: + ``` + + ``` + + - Data source specific features are allowed. Not all data source features will be generic in nature, but that shouldn't prevent us from exposing them to make it easy for customers to utilize the feature. -1. Tests names should be descriptive and from a users' perspective: - - *GOOD NAME EXAMPLE*: “it creates new user when new user form is submitted” - - *BAD NAME EXAMPLE*: “button dispatches onClick event” + - Components should try to stay data source generic whenever possible. + - Example: A line chart is quite generic and should support any time series data. + + - Components must provide a user understandable message when provided unsupported data types. + +- #### Exports + - Web components will be exported within https://github.com/awslabs/iot-app-kit/tree/main/packages/components, with a iot- prefix. i.e. iot-line-chart. Their styles should be contained within `"@iot-app-kit/components/styles.css"`. + + - React components will be exported within https://github.com/awslabs/iot-app-kit/tree/main/packages/react-components with no prefix. i.e. LineChart -1. Tests should, at minimum, cover all core feature use-cases and identifiable edge-cases or boundary conditions. + - Components can also be created in other repositories, but must ultimately be exported from either @iot-app-kit/components and @iot-app-kit/react-components (or both). -1. Code must be written in a consistent style, utilizing the shared eslint and stylelint configurations https://github.com/awslabs/iot-app-kit/blob/main/.eslintrc.js https://github.com/awslabs/iot-app-kit/blob/main/.stylelintrc -1. Pull requests to feature branches must contain only a single, well formatted commit that adheres to https://www.conventionalcommits.org/en/v1.0.0/ -1. Pull requests to main must have correct change logs which properly document migrations, feature changes, and most importantly breaking change. [example commit](https://github.com/awslabs/iot-app-kit/commit/9e09d6229dc5d27309766db4717d5a250e91bdd7) -1. Pull requests to main from a feature branch should maintain the git history as contained in the feature branch -1. Pull requests to main must have the correct versioning updates, following https://semver.org/ -1. Pull request descriptions should include a reasonably detailed summary of changes and highlight any architectural decisions introduced. -1. Reviewers are expected to enforce a high standard on: - 1. Consistent code to rest of codebase - 1. High quality tests that are not painful to maintain - 1. Architectural and big picture aspects of how the change fits in -1. It is ok to build out work incrementally within a feature branch, but not in main. +- #### Visualization (Viewport) + - Components that visualize time series data must support a viewport. + - If the viewport contains a duration the component should visualize the last duaration ms. - For example, it is OK to have a PR merged into a feature branch with missing test cases and add those in the next PR. This helps allow people to get code in more frequently rather than having long lived local branch. + - If the viewport contains start and end dates the component should visualize data between the start and end. - When a pull request to main is made, there will be no “rolling fixes/improvements in next PR”. -12. Avoid long-lived local feature-branches. Make pull requests incrementally, even if not everything is perfect. Explicitly state in the PR overview what aspects are in progress to help reviewers effectively understand what parts to provide feedback on. -13. Customer facing APIs should not require ENUMS. Strings with strict typescript types are preferable and easier for customers to utilize. + - Example: + ``` + + ``` - *GOOD EXAMPLE*: +--- + +## Code style + +- Code must be written in a consistent style, utilizing the shared [eslint](https://github.com/awslabs/iot-app-kit/blob/main/.eslintrc.js) and [stylelint](https://github.com/awslabs/iot-app-kit/blob/main/.stylelintrc) configurations. + +- Customer facing APIs should not require ENUMS. Strings with strict TypeScript types are preferable because they are easier for customers to utilize. - ``` + - *GOOD EXAMPLE*: + ``` ``` - - *BAD EXAMPLE*: - + - *BAD EXAMPLE*: ``` ``` -14. Make the code consistent with the existing code base -15. Avoid mutations when possible. Prefer immutability +- Make the code consistent with the existing code base. - *GOOD EXAMPLE*: +- Avoid mutations when possible. Prefer immutability. - Returns updated object without mutating the input - + - *GOOD EXAMPLE*: Returns updated object without mutating the input. ``` (myObject) => ({ ...myObject, name: trimName(myObject.name (http://myobject.name/)) }) ``` - *BAD EXAMPLE*: - - Mutates input to method (DO NOT DO THIS) - + - *BAD EXAMPLE*: Mutates input to method. ``` (myObject) => { myObject.name (http://myobject.name/) = trimName(myObject.name) } ``` -16. Keep it simple - - Code bases tend to only grow in complexity, until no one can understand them or maintain them. It is paramount that reducing complexity is a top concern on every piece of code. - -17. Write functions as pure functions where possible. +- Keep it simple. Code bases tend to only grow in complexity, until no one can understand them or maintain them. It is paramount that reducing complexity is a top concern on every piece of code. - A pure function is a function which takes inputs, and returns a result, without modifying the inputs and without any side effects. +- Write functions as pure functions where possible. A pure function is a function which takes inputs, and returns a result, without modifying the inputs and without any side effects. - *EXAMPLE OF PURE FUNCTION*: - + - *EXAMPLE OF PURE FUNCTION*: ``` (a, b) => a + b ``` - - *EXAMPLE OF IMPURE FUNCTION*: - + - *EXAMPLE OF IMPURE FUNCTION*: ``` () => { globalCounter++; } ``` Impure functions are unavoidable, but should be minimized, and should contain the minimal required. Complicated business logic should be separated off into independent pure functions. Pure functions are much easier to test, and utilize. + +- Consider performance where it matters. In some scenarios, improving the performance will add complexity, while not actually effecting the end performance seen by the users. In this case, always opt for simplicity. -18. Consider performance where it matters +- Embrace refactoring. Refactoring is crucial to the health of any code base. Fearlessly refactor where it can help improve simplicity. Often times code when originally written misses the mark. Do not be afraid to refactor code written by other authors. - In some scenarios, improving the performance will add complexity, while not actually effecting the end performance seen by the users. In this case, always opt for simplicity. +- State is the root of complexity. Minimize the amount of state is required and avoid entangling state management through the entire codebase. Often times state should be pushed towards the boundary (such as in a reducer). The being said there’s nothing wrong with local UI state managed directly in a component. For example, for a collapsable menu, it will probably make sense to just store the open state directly in the component. Do not blindly pull all state into a centralized state manager as a reducer. -19. Embrace refactoring + - *EXAMPLE OF BAD STATE MANAGEMENT*: + UI components storing state of multiple async calls, coordinating loading status, and pagination. +--- - Refactoring is crucial to the health of any code base. Fearlessly refactor where it can help improve simplicity. Often times code when originally written misses the mark. Do not be afraid to refactor code written by other authors. -20. State is the root of complexity +## Documentation - Minimize the amount of state is required and avoid entangling state management through the entire codebase. Often times state should be pushed towards the boundary (such as in a reducer). +- Documentation must be written in a style that is consistent with the rest of the documentation. Use the [AWS documentation style](https://docs.aws.amazon.com/iot-sitewise/) as a guide, as well as cross-referencing the other [iot-app-kit documentation](https://github.com/awslabs/iot-app-kit/tree/main/docs). - The being said there’s nothing wrong with local UI state managed directly in a component. For example, for a collapsable menu, it will probably make sense to just store the open state directly in the component. Do not blindly pull all state into a centralized state manager as a reducer. +- Ensure that all documentation is correctly referenced in the (table of contents)[https://github.com/awslabs/iot-app-kit/blob/main/README.md]. - *EXAMPLE OF BAD STATE MANAGEMENT*: +- Review the [in-depth guidelines](https://alpha-docs-aws.amazon.com/awsstyleguide/latest/styleguide/awsstyleguide.pdf#aws-implementation-guides) on documentation. This can be a useful reference, but you are not expected to read through and be 100% adherent to these guidelines. - UI components storing state of multiple async calls, coordinating loading status, and pagination +--- -## IoT App Kit component requirements +## Pull requests -1. Components will utilize [Cloudscape](https://cloudscape.design/) for primitive components, unless a good case is made not to. The default is to use Cloudscape. -1. Styling should adhere to https://cloudscape.design/ design principles and guidelines. -1. Web components will be exported within https://github.com/awslabs/iot-app-kit/tree/main/packages/components, with a iot- prefix. i.e. iot-line-chart. Their styles should be contained within `"@iot-app-kit/components/styles.css"`. -1. React components will be exported within https://github.com/awslabs/iot-app-kit/tree/main/packages/react-components with no prefix, i.e. LineChart -1. Components can also be created in other repositories, but must ultimately be exported from either @iot-app-kit/components and @iot-app-kit/react-components (or both) -1. All components prior to release must have public facing documentation within https://github.com/awslabs/iot-app-kit/tree/main/docs, including updating the https://github.com/awslabs/iot-app-kit with the relevant information. -1. Components must work across the commonly used browsers. Any browser that has over 1% of total internet usage as defined by https://caniuse.com/usage-table is considered a commonly used browser. (which does not include IE11. +- Pull requests to feature branches must contain only a single, well formatted commit that adheres to [conventional commits](https://www.conventionalcommits.org/en/v1.0.0/). -1. All IoT App Kit components contain a `size` property, which can be "XS", "S", "M", "L", "XL", or "XXL". +- Pull requests to main must have correct change logs which properly document migrations, feature changes, and most importantly breaking change. [example commit](https://github.com/awslabs/iot-app-kit/commit/9e09d6229dc5d27309766db4717d5a250e91bdd7) - These sizes do not affect the overall width and height of the element, but instead form a guide for sizing widget elements such as font size, line thickness, symbol size, and level of detail. Each size has a standard sizing for various elements, as well as a dimension which the widget should look good at. For example, an XS widget should be displayable at a size of 75px by 75px, but a user could display a XS widget in a larger container if they desired to. However a XXL widget if put in a small container, will not work well. +- Pull requests to main from a feature branch should maintain the git history as contained in the feature branch. - | Size | Primary font size | Large font size |Icon size | Looks good at dimension | - |------|-------------------|-----------------|----------| ------------------------| - | XS | 14 | 20 | 16 | 75px by 75px | - | S | 16 | 24 | 20 | 100px by 100px | - | M | 20 | 32 | 32 | 150px by 150px | - | L | 24 | 48 | 48 | 200px by 200px | - | XL | 32 | 60 | 60 | 300px by 300px | - | XXL | 48 | 96 | 96 | 500px by 500px | +- Pull requests to main must have the correct versioning updates following [semver](https://semver.org/). - (Note: this specification is not exhaustive and subject to change. Size-affected elements will vary by component +- Pull request descriptions should include a reasonably detailed summary of changes and highlight any architectural decisions introduced. -1. Components must support internationalization via messageOverrides (i.e. avoid hard coded phrases) +- Reviewers are expected to enforce a high standard on: + - Consistent code to rest of codebase. + - High quality tests that are not painful to maintain. + - Architectural and big picture aspects of how the change fits in. - for example, - +- It is ok to build out work incrementally within a feature branch, but not in main. For example, it is OK to have a PR merged into a feature branch with missing test cases and add those in the next PR. This helps allow people to get code in more frequently rather than having long lived local branch. When a pull request to main is made, there will be no “rolling fixes/improvements in next PR”. + +- Avoid long-lived local feature-branches. Make pull requests incrementally, even if not everything is perfect. Explicitly state in the PR overview what aspects are in progress to help reviewers effectively understand what parts to provide feedback on. +--- + +## Tests + +- Tests should be written to maximize the chance that the test would actually catch a real issue, i.e. the likelihood of preventing a regression. This is the primary value of a test. Useful talk on testing: https://www.youtube.com/watch?v=Fha2bVoC8SE3. + +- Tests should be written to minimize the pain they inflict on the development cycle. Every test represents some amount of burden, through the necessity to run it, maintain it, and alter the test as the codebase morphs, however some tests inflict more pain than others. + + - *EXAMPLES OF PAIN*: + - tests failing when nothing broke + - flaky tests + - snapshot tests that vary on machine + - tests that require refactoring when irrelevant portions of the codebase are altered + +- Tests should be written to not fail when things are not broken (this helps reduce inflicted pain). This can be achieved through techniques such as not over asserting. + + - *GOOD ASSERTION EXAMPLE*: ``` - + expect(result).toEqual(expect.objectContaining({ error: null }) ``` -1. Components should support various data sources, via either a query or queries property. - for example, + - *BAD ASSERTION EXAMPLE*: every test asserting on every property over and over whether or not they are relevant to the purpose of the test + +- Tests names should be descriptive and from a users' perspective. + + - *GOOD NAME EXAMPLE*: + ``` + it("creates new user when new user form is submitted”) + ``` - ``` - - ``` -1. It is ok to have data-source specific features. Not all data-source features will be generic in nature, but that shouldn’t prevent us from exposing them to make it easy for customers to utilize the feature. -1. Components should try there best to work with a generic data-source where it makes sense. - - For example, a line chart component is quite generic and should be able to support any time series data. -1. Components that visualize time series data, must support a viewport, -1. If the viewport contains a duration, then the component should properly interpret this as visualizing the last duration ms -1. If the viewport contains a start and an end date, the component should properly interpret this as visualizing the data between start and end. - - for example, the following line chart should display the last 1 seconds of data queried - ``` - - ``` -1. Components must provide a user understandable message when provided unsupported data types -1. Components must properly handle loading states -1. Components must properly handle error states -1. Components must support annotations where they make sense. An example of an annotation can be found at https://synchrocharts.com/#/Features/Annotation -1. Components must visualize trend lines where it makes sense, for example, a line chart should visualize the trend line. An example of a trend line can be found at https://synchrocharts.com/#/Features/Trends + - *BAD NAME EXAMPLE*: + ``` + it(“button dispatches onClick event”) + ``` + +- Tests should, at minimum, cover all core feature use-cases and identifiable edge-cases or boundary conditions. \ No newline at end of file