Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Maintainability Items: linting and typing #69

Merged
merged 9 commits into from
Dec 5, 2022
Merged

Maintainability Items: linting and typing #69

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3014fe3
deps: add and configure typescript
klardotsh Nov 23, 2022
ecdacd5
deps: Install and configure ESLint based on XO defaults.
klardotsh Nov 24, 2022
c09b4a0
types: Provide basic structural type hints for zulip-js.
klardotsh Nov 24, 2022
1d7707f
send-message: Port action to strict TypeScript.
klardotsh Nov 24, 2022
79c1ebc
markdown: Lint and fix with prettier.
klardotsh Nov 29, 2022
4cfd4b7
docs: Add contribution guidelines based on zulip/zulip.
klardotsh Nov 29, 2022
aac372e
docs: Reformat send-message documentation into a condensed table.
klardotsh Nov 29, 2022
dc7e6c7
docs: Document a bot-specific note about which email address to use.
klardotsh Nov 30, 2022
7dadd3d
docs: Clarify required state of topic parameter.
klardotsh Nov 30, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 16 additions & 0 deletions .eslintrc.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
{
"extends": [
"xo",
"xo-typescript",
"plugin:import/recommended",
"plugin:import/typescript",
"plugin:unicorn/recommended",
"plugin:prettier/recommended"
],
"root": true,
"rules": {
// These are incompatible with TypeScript "module: commonjs"
"@typescript-eslint/no-floating-promises": "off",
"unicorn/prefer-top-level-await": "off"
}
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to apply eslint --fix in this commit? Usually we do that when configuring a linter.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After eslint --fix we'll still have 16 non-auto-fixable (and frankly non-trivial even with human intervention) linter errors. I'd wager probably not worth rolling back to try to fix that since the code being fixed gets fully replaced in the next commits anyway, but that's not a hill I'd die on or anything.

(woods) github-actions-zulip  » (0cfc8ee...) » git checkout 0cfc8eec86b7719aed832f13bd3528c92615d5ef; npm ci; npx eslint --fix ./src/send-message.js 
HEAD is now at 0cfc8ee deps: Install and configure ESLint based on XO defaults.

added 278 packages, and audited 279 packages in 3s

# snip...

/home/j/src/zulip/github-actions-zulip/src/send-message.js
  29:13  error  Unsafe assignment of an `any` value                @typescript-eslint/no-unsafe-assignment
  29:31  error  Unsafe call of an `any` typed value                @typescript-eslint/no-unsafe-call
  29:50  error  Unsafe return of an `any` typed value              @typescript-eslint/no-unsafe-return
  29:50  error  Unsafe call of an `any` typed value                @typescript-eslint/no-unsafe-call
  31:9   error  Unsafe assignment of an `any` value                @typescript-eslint/no-unsafe-assignment
  31:14  error  Unsafe call of an `any` typed value                @typescript-eslint/no-unsafe-call
  49:11  error  Unsafe assignment of an `any` value                @typescript-eslint/no-unsafe-assignment
  49:26  error  Unsafe call of an `any` typed value                @typescript-eslint/no-unsafe-call
  57:11  error  Unsafe assignment of an `any` value                @typescript-eslint/no-unsafe-assignment
  57:28  error  Unsafe call of an `any` typed value                @typescript-eslint/no-unsafe-call
  60:50  error  Invalid type "any" of template literal expression  @typescript-eslint/restrict-template-expressions
  62:13  error  Unsafe assignment of an `any` value                @typescript-eslint/no-unsafe-assignment
  63:14  error  Invalid type "any" of template literal expression  @typescript-eslint/restrict-template-expressions
  63:32  error  Invalid type "any" of template literal expression  @typescript-eslint/restrict-template-expressions
  67:5   error  Implicit any in catch clause                       @typescript-eslint/no-implicit-any-catch
  75:5   error  Implicit any in catch clause                       @typescript-eslint/no-implicit-any-catch

✖ 16 problems (16 errors, 0 warnings)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to go back to the add-linting commit anyway to address a commit cleanliness comment from Alex so I met in the middle here if that's cool with you: ran lint:fix over the original JS in the commit that adds eslint-config-xo and company, but does not resolve the outstanding 18 (up from 16 because I'd missed eslint-config-xo not being inherently pulled in by eslint-config-xo-typescript, oops) non-auto-fixable errors. It does, however, resolve about 50 auto-fixable errors at that point in the commit history.

That's all now implemented in ecdacd5

15 changes: 14 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,4 +6,17 @@ This is a collection of GitHub Actions to interact with [Zulip](https://zulip.co

## Actions

- [Sends a message to Zulip](./send-message/README.md)
- [`zulip/github-actions-zulip/send-message@v1` sends a message to Zulip](./send-message/README.md)

## Contributing

These actions are implemented in TypeScript, see
[`tsconfig.json`](./tsconfig.json) for (generally strict mode defaults)
compiler settings. Use `npm run lint:fix` to autoformat all TypeScript and
Markdown to the expected style. Use `npm run package` to generate
`dist/**/*.js`, never edit those files by hand. Commits should follow [the
Zulip Commit Discipline
guide](https://zulip.readthedocs.io/en/latest/contributing/version-control.html),
which can then be contributed upstream to this repository following the [Zulip
Reviewable Pull Requests
guide](https://zulip.readthedocs.io/en/latest/contributing/reviewable-prs.html)
Loading