Skip to content

Commit f1ce1c4

Browse files
jeremymengCopilot
andauthored
[Docs] update contributing guides (#33386)
- add a linting.md file to describe our linting workflow - add a doc about resolving pnpm-lock merge conflict in PRs - remove some outdated info --------- Co-authored-by: Copilot <[email protected]>
1 parent 9c5e2e8 commit f1ce1c4

File tree

4 files changed

+64
-10
lines changed

4 files changed

+64
-10
lines changed

.github/copilot-instructions.md

+4
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ You are a highly experienced engineer with expertise in
1212

1313
- Always ensure your solutions prioritize clarity, maintainability, and testability.
1414
- Never suggest re-recording tests as a fix to an issue
15+
- NEVER turn off a rule in `eslint-plugin-azure-sdk` plugin to resolve linting issues.
1516
- Always review your own code for consistency, maintainability, and testability
1617
- Always ask how to verify that your changes are correct, including any relevant tests or documentation checks.
1718
- Always ask for clarifications if the request is ambiguous or lacks sufficient context.
@@ -44,6 +45,7 @@ Always attempt to browse the following resources and incorporate relevant inform
4445
- https://github.com/Azure/azure-sdk/blob/main/docs/policies/repostructure.md
4546
- https://azure.github.io/azure-sdk/typescript_introduction.html
4647
- https://github.com/Azure/azure-sdk-for-js/blob/main/documentation/Quickstart-on-how-to-write-tests.md
48+
- linting: https://github.com/Azure/azure-sdk-for-js/blob/main/documentation/linting.md
4749

4850
When reviewing documentation URLs (especially Azure SDK documentation), extract key points, principles, and examples to inform your responses.
4951
Always cite the specific sections of documentation you've referenced in your responses.
@@ -68,6 +70,8 @@ In general, whenever a code refers to `@azure/core-*` packages, we will expect c
6870

6971
If a change requires updates to the core packages, you will remind the user to run `rush build -t .` commands.
7072

73+
Refer to `rush.json` if you need to resolve a package directory from its package name.
74+
7175
## Azure SDK Guidelines
7276

7377
Generate TypeScript code that adheres strictly to these guidelines.

CONTRIBUTING.md

+10-10
Original file line numberDiff line numberDiff line change
@@ -107,7 +107,7 @@ To build all packages:
107107
To build specific package(s), use `-t` rush command-line option:
108108

109109
6. Install and link all dependencies (`rush update`)
110-
7. Build the package, for example, `rush build -t @azure/service-bus`. Alternatively when under the package folder, `rush build -t .`
110+
7. Build the package, for example, `rush build -t @azure/service-bus`. Alternatively when under the package directory, `rush build -t .`
111111

112112
## Development Workflows
113113

@@ -125,6 +125,8 @@ If you manually edit dependencies within the package.json for any reason, make s
125125

126126
Any time you add, update, or remove dependencies, running `rush update` will generate a diff to the file `common/config/rush/pnpm-lock.yaml`. You should commit these changes - this file works similarly to NPM's package-lock.json files, except it tracks package versions for all projects in the Rush workspace. Do not check in any package-lock.json files.
127127

128+
Because multiple pull requests may be changing `pnpm-lock.yaml` at the same time, it is very common that the first merged one will cause merge conflicts for the later ones. Please refer to [the instructions](https://github.com/Azure/azure-sdk-for-js/blob/main/documentation/resolve-pnpm-lock-merge-conflict.md) on resolve PR merge conflicts for `common/config/rush/pnpm-lock.yaml`
129+
128130
### Resolving dependency version conflicts
129131

130132
When you run `rush update`, Rush will also ensure that dependency versions are consistent across all of our packages. If they are not, the command will fail and show you all packages which use a conflicting versions of dependencies. There are a few ways to resolve this:
@@ -251,13 +253,13 @@ In the case where you do not want to generate documentation for a specific defin
251253
To maintain the quality of the documentation, the following two facilities are provided:
252254

253255
- an [ESLint plugin](https://github.com/microsoft/tsdoc/tree/master/eslint-plugin) is used to check that our comments are well-formed TSDoc comments and it can be run using `rushx lint`
254-
- the documentation can be generated locally for a particular package using `rushx docs` and it can be inspected by opening `sdk/<package path>/dist/docs/index.html` in your favorite browser
256+
- documentation artifacts are generated in pull request checks Azure Pipelines. They can be downloaded, extracted to local disk, and inspected by opening `azure-<package name>/<version>/index.html` in your favorite browser. Click on "xx published; xx consumed" under **Related**, expand packages > azure-xxxxx > documentation then download the azure-xxxxx.zip file.
255257

256258
TSDoc specifications can be customized using the `tsdoc.json` configuration file that can be found in the root of the repository. Currently, the `@hidden` tag is used which is only supported by TypeDoc and is not a TSDoc tag, so it is added as a custom tag in `tsdoc.json`.
257259

258260
### Formatting changed files
259261

260-
We used to have a git hook that formats your changed files on commit but it was removed because it did not work well for some people for various reasons. If you would like to enable it in your fork, you will need to just revert this [PR](https://github.com/Azure/azure-sdk-for-js/pull/13982/) in your branch and then run `rush update` so the hook script gets copied into `.git/hooks`. Moreover, without the hook, you can manually format changed files by invoking `rush prettier`.
262+
We used to have a git hook that formats your changed files on commit but it was removed because it did not work well for some people for various reasons. If you would like to enable it in your fork, you will need to just revert this [PR](https://github.com/Azure/azure-sdk-for-js/pull/13982/) in your branch and then run `rush update` so the hook script gets copied into `.git/hooks`. Moreover, without the hook, you can manually format changed files by invoking `rushx format` under your package directory.
261263

262264
### Enforcing Azure SDK design guidelines
263265

@@ -266,18 +268,16 @@ Our libraries follow the [TypeScript SDK design guidelines](https://azure.github
266268
- [add `eslint` to your `devDependencies`](https://github.com/Azure/azure-sdk-for-js/blob/8ec9801c17b175573a115fc8b2d6cbaeb17b0b09/sdk/template/template/package.json#L106)
267269
- [add `eslint-plugin-azure-sdk` to your `devDependencies`](https://github.com/Azure/azure-sdk-for-js/blob/8ec9801c17b175573a115fc8b2d6cbaeb17b0b09/sdk/template/template/package.json#L93)
268270
- add a linting npm script as follows:
269-
- ["lint": "eslint package.json api-extractor.json src test --ext .ts"](https://github.com/Azure/azure-sdk-for-js/blob/8ec9801c17b175573a115fc8b2d6cbaeb17b0b09/sdk/template/template/package.json#L49)
271+
- ["lint": "eslint package.json api-extractor.json src test"](https://github.com/Azure/azure-sdk-for-js/blob/8ec9801c17b175573a115fc8b2d6cbaeb17b0b09/sdk/template/template/package.json#L49)
270272

271-
You can run the plugin by excuting `rushx lint` inside your package directory.
273+
You can run the plugin by executing `rushx lint` inside your package directory. You need to build the plugin at least once either directly via `rush build -t eslint-plugin-azure-sdk`, or indirectly as your package's dependency by `rush build -t .` under your package directory.
272274

273-
If the package is internal, it should not follow the design guidelines and in turn should not be linted by the plugin. In this case, use the an internal config from `eslint-plugin-azure-sdk` instead. For example: `"lint": "eslint src test"` with the following eslint.config.mjs
275+
If the package is internal, it should not follow the design guidelines and in turn should not be linted using the same set of rules. In this case, use the an internal config from `eslint-plugin-azure-sdk` instead. For example: `"lint": "eslint src test"` with the following eslint.config.mjs
274276

275277
```javascript
276278
import azsdkEslint from "@azure/eslint-plugin-azure-sdk";
277279

278-
export default [
279-
...azsdkEslint.configs.internal,
280-
];
280+
export default [...azsdkEslint.configs.internal];
281281
```
282282

283283
## Onboarding a new library
@@ -290,7 +290,7 @@ The second type of libraries is more complex to develop and maintain because the
290290

291291
Rush assumes that anything printed to `STDERR` is a warning. Your package scripts should avoid writing to `STDERR` unless emitting warnings or errors, since this will cause Rush to flag them as warnings during the execution of your build or script command. If your library uses a tool that can't be configured this way, you can still append `2>&1` to the command which will redirect all output to `STDOUT`. You won't see warnings show up, but Rush will still consider the command to have failed as long as it returns a nonzero exit code.
292292

293-
In general, it's recommended to avoid using NPM [hook scripts](https://docs.npmjs.com/misc/scripts) (those starting with `pre` / `post`). The build system will always explicitly run the `install`, `build`, `build:test`, `pack`, `audit`, `lint`, `unit-test`, and `integration-test` scripts at the appropriate times during the build. Adding hooks that performs steps like installing dependencies or compiling the source code will at best slow down the build, and at worst may lead to difficult to diagnose build failures.
293+
In general, it's recommended to avoid using NPM [hook scripts](https://docs.npmjs.com/misc/scripts) (those starting with `pre` / `post`). The build system will always explicitly run the `install`, `build`, `build:test`, `pack`, `lint`, `unit-test`, and `integration-test` scripts at the appropriate times during the build. Adding hooks that perform steps like installing dependencies or compiling the source code will at best slow down the build, and at worst may lead to difficult to diagnose build failures.
294294

295295
Because Rush uses PNPM to download and manage dependencies, it's **_especially_** important to make sure that none of your package scripts are calling `npm install` when your library is built via the Rush toolchain. Most commonly this occurs in a `prepack` or `prebuild` script. Ensure your library does not contain these scripts - or if you determine that such a script is required, ensure that it doesn't run `npm install`.
296296

documentation/linting.md

+33
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
We use recommended rules from `eslint`, `typescript-eslint`, and a set of our custom rules to ensure our code adhere to the Azure SDK design guidelines and have high quality.
2+
3+
In this document, we describe how to address common linting issues for a package.
4+
5+
# Azure SDK eslint plugin
6+
7+
Our custom linting rules and recommended configurations is hosted in the `eslint-plugin-azure-sdk` package. This package needs to be built first before linting any SDK packages.
8+
9+
- `rush update`
10+
- `rush build -t eslint-plugin-azure-sdk`
11+
12+
It also gets built as a dependency of any SDK packages.
13+
14+
# Linting a package
15+
16+
This is done by running `rushx lint` under the package directory.
17+
18+
1. `cd sdk/<service-directory>/<package-directory>` if your current directory is not the package directory yet.
19+
2. `rushx lint`
20+
21+
# Fixing linting issues
22+
23+
You should NEVER turn off a rule in `eslint-plugin-azure-sdk` to resolve linting issues.
24+
25+
Some linting rules provide auto fixer. To use them, run `rushx lint:fix` under the package directory.
26+
27+
For linting issues that `lint:fix` script could not resolve, you will need to examine the code and fix the issues accordingly.
28+
29+
For documentation on `eslint` rules, refer to https://eslint.org/docs/latest/rules/.
30+
31+
For documentation on `typescript-eslint` rules, refer to https://typescript-eslint.io/rules/
32+
33+
For a generated package whose name starts with `@azure/arm-` or `@azure-rest/`, there might be "tsdoc/syntax" warnings because generated code files often contains some characters in reference docs that are not recommended in TSDoc. We should NEVER fix auto-generated files. You can add rules to suppress the "tsdoc/syntax" rules in the package's ESLint configuration file eslint.config.mjs. Create that file by using one from other packages as a template but do not copy rules that don't apply.
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
Here's the recommended process to resolve merge conflicts of `common/config/rush/pnpm-lock.yaml` in your pull request (assuming your upstream remote is named "upstream"):
2+
3+
0. Find out your upstream remote name for `Azure/azure-sdk-for-js` repository: `git remote -v`
4+
1. Get the latest changes from the upstream main branch: `git fetch upstream main`
5+
2. Merge the latest changes from main to local pull request branch: `git merge --no-edit upstream/main`
6+
3. Use your favorite IDE, editor, or Git Client to resolve conflicts for files other than `common/config/rush/pnpm-lock.yaml`
7+
4. Check out the main branch version of pnpm-lock.yaml: `git checkout upstream/main -- common/config/rush/pnpm-lock.yaml`
8+
5. Refresh dependencies: `rush update`
9+
6. Stage the updates: `git add common/config/rush/pnpm-lock.yaml`
10+
7. Commit the merge
11+
8. Push the commit to your pull request branch, assuming "origin" is the git remote name of your fork, `git push origin`
12+
13+
Step 4) to 6) can be combined and enhanced to run under any directory under the repo if you are using a \*NIX environment:
14+
15+
```shell
16+
git checkout upstream/main `git rev-parse --show-toplevel`/common/config/rush/pnpm-lock.yaml && rush update && git add `git rev-parse --show-toplevel`/common/config/rush/pnpm-lock.yaml
17+
```

0 commit comments

Comments
 (0)