From b84d47c2da1cb715c12355c9afe2c8eecebdae43 Mon Sep 17 00:00:00 2001 From: "sweep-ai[bot]" <128439645+sweep-ai[bot]@users.noreply.github.com> Date: Sun, 29 Oct 2023 21:39:26 +0000 Subject: [PATCH] feat: Updated CONTRIBUTING.md --- CONTRIBUTING.md | 40 +++++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 19 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 5d45f7611d..afdb4b3729 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -14,6 +14,10 @@ Squiggle is currently in "Early Access" mode. Anyone (with a GitHub account) can file an issue at any time. Please allow [Slava](https://github.com/berekuk) and [Ozzie](https://github.com/OAGr) to triage, but otherwise just follow the suggestions in the issue templates. +# Code Quality + +- Aim for at least 8/10 quality in `/packages/squiggle-lang`, and 7/10 quality in `/packages/components`. + # Project structure Squiggle is a **monorepo** with several **packages**. @@ -53,38 +57,38 @@ Autopings are set up: if you are not autopinged, you are welcome to comment, but # Code Quality -- Aim for at least 8/10\* quality in `/packages/squiggle-lang`, and 7/10 quality in `/packages/components`. +- Aim for at least 8/10 quality in `/packages/squiggle-lang`, and 7/10 quality in `/packages/components`. - If you submit a PR that is under a 7, for some reason, describe the reasoning for this in the PR. * This quality score is subjective. # TypeScript style -**Prefer `const` over `let`, never use `var`** +**Prefer `const` over `let`, and never use `var`** `var` is deprecated in JS. `let` should only be used for mutable variables. -**Use functional style, avoid classes** +**Use functional style and avoid classes whenever possible** We use classes for outer-facing APIs, but most of the codebase should use plain immutable objects with functions act on those objects. -**Use immutable types when it doesn't hurt the performance** +**Wrap object types in [Readonly](https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype) or mark individual fields as `readonly`** -Wrap object types in [Readonly](https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype) or mark individual fields as `readonly`. +Use immutable types when it doesn't hurt the performance. Wrap object types in [Readonly](https://www.typescriptlang.org/docs/handbook/utility-types.html#readonlytype) or mark individual fields as `readonly`. -**Don't use namespaces** +**Use native ES modules instead of namespaces, as recommended by TypeScript documentation** -Use native ES modules instead, as [recommended by TypeScript documentation](https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html#using-modules). +Don't use namespaces. Use native ES modules instead, as [recommended by TypeScript documentation](https://www.typescriptlang.org/docs/handbook/namespaces-and-modules.html#using-modules). -**Avoid `any` as much as possible** +**Avoid using `any` as much as possible** -It's almost always possible to type things properly with modern Typescript. +It's almost always possible to type things properly with modern Typescript. Avoid using `any` as much as possible. -**Always use `===` instead of `==`** +**Always use `===` instead of `==` for strict equality comparison** -Loose equality is [crazy](https://dorey.github.io/JavaScript-Equality-Table/unified/). +Loose equality is [crazy](https://dorey.github.io/JavaScript-Equality-Table/unified/). Always use `===` instead of `==` for strict equality comparison. -**Don't use too many external libraries** +**Avoid using too many external libraries, especially in `squiggle-lang`** This is especially important in `squiggle-lang` and less important in `website` or some other code that won't be used much as a library by many users. @@ -96,17 +100,15 @@ Heuristics for deciding whether pulling an external library is worth it: - would we want to fine-tune the implementation in the future, because of Squiggle's design needs or for the sake of performance? - the closer to the "core" functionality of Squiggle the feature is (math, distributions-related code), the more it makes sense to keep control over the implementation details -**Prefer named exports over default exports** +**Prefer named exports over default exports for easier refactorings** It's easier to do refactorings with named exports. -**Name files according to their main named exports; split code into many small files** - -This is expecially straightforward in the frontend code; try to put one component in a single file, `export const MyComponent: FC = ...` from `MyComponent.tsx`. +**Name files according to their main named exports and split code into many small files for better organization** -In the squiggle-lang code, I'm not sure yet if this is viable. +This is especially straightforward in the frontend code; try to put one component in a single file, `export const MyComponent: FC = ...` from `MyComponent.tsx`. In the squiggle-lang code, I'm not sure yet if this is viable. -**Prefer `type` over `interface`** +**Prefer `type` over `interface` for consistency and avoid open interfaces by default** In the modern TypeScript there's no [big](https://stackoverflow.com/questions/37233735/interfaces-vs-types-in-typescript/52682220) [difference](https://www.typescriptlang.org/docs/handbook/2/everyday-types.html#differences-between-type-aliases-and-interfaces) between types and interfaces. Errors in interfaces can be slightly nicer, but interfaces are open by default and we mostly don't want that. @@ -116,7 +118,7 @@ It's not worth fighting over, though, the difference is pretty small. Exceptions: -- For React components, it's better to use `const my Component: FC<...> = ` since it type-checks both the arguments and the result type. +- For React components, it's better to use `const myComponent: FC<...> = ` since it type-checks both the arguments and the result type. - In some other cases where you have several functions with the similar signature, it's also convenient to define a type for the entire function, and you have to use arrow functions to use it. Also, don't use `function` keyword in non-top scopes (it's easier to avoid hoisting quirks this way).