Maintainability Items: linting and typing #69

klardotsh · 2022-11-24T04:02:17Z

As mentioned on CZO, I figured I'd take a pass at tidying up this codebase before adding more in-house uses of it in some future work. As such, this PR sets up linting (based on xojs/xo's defaults, largely similar to the style in zulip/zulipbot), and refactors the existing send-message action into a strict and functional flavor of TypeScript.

I'd originally intended to just run the existing code through the TypeScript compiler, see what complained, touch those up, and move on, but after realizing I hated the type annotation of string | number | (string|number)[] for the heavily-reassigned to variable, and also that validation/business logic was spread all around the function in difficult-to-trace ways, I went for a more comprehensive refactor.

Opening as a draft to get high-level feeedback on approaches within (particularly, the use of the heavily-Rust-inspired ts-results and ts-async-results helper libraries to get a Result/Maybe monad) while I dig into testing this with my own bot users: as it stands this code passes type checking, but I have not actually run the action in a GitHub Actions environment to regression-test the functionality. update: https://github.com/klardotsh/zulip-gha-playground/actions/runs/3579426796 passes a simple regression test.

The zulip-js.d.ts stubs are purely structural hints: I don't attempt to model the responses from the server, I simply model a subset of the Zulip client itself. As it stands the hints are incomplete enough as to potentially be confusing if integrated to zulip/zulip-js, so I've kept them separate (despite the pitfalls regarding the Response objects mentioned in a comment). I'm open to discussing a proper plan for mainlining those to the proper repo and what that process would look like, or to punting that discussion for later.

klardotsh · 2022-11-24T04:22:39Z

src/send-message.ts

+    default: {
+      assertExhaustiveSwitch(kind);
+    }


In case someone reviewing isn't a TypeScript person: this trick will detect any unhandled (or, coincidentally, un-terminated) branches of the enum switch and turn them into compiler errors (familiar to Rust folks, probably other languages too), since never is a bottom type.

src/send-message.ts

klardotsh · 2022-11-24T04:26:11Z

src/send-message.ts

+        new AsyncResultWrapper(
+          Result.wrapAsync(async () =>
+            zulipInit({
+              apiKey,
+              username,
+              realm,
+            })
+          )
+        )


Frankly this interaction between ts-async-results and the parent library, ts-results, looks pretty gross. There may be a simpler way to do this, but after reading upstream issues in both repos and a lengthy conversation with the type checker, this is the best I came up with so far.

klardotsh · 2022-11-24T04:29:06Z

src/send-message.ts

+    Result.all(
+      getMandatoryInputFromJob("content"),
+      getDestinationDetails(),
+      await getZulipClient()
+    )


It may make sense to instead use AsyncResult.all, though the ergonomics are a little weirder there. It would probably replace the manual call to new AsyncResultWrapper at the top level, though may create two new calls to it on L144+L145. I'll do some investigation on this Monday.

I assume that using [AsyncResult.all](https://github.com/movesthatmatter/ts-async-results#combining-results) didn't bear any fruit?

alexmv

Thanks for all of the work converting this! I'm excited to have this be more up to the standards of zulip/zulip.

I'm pretty non-qualified for the TS parts of this, but I left a couple comments on things that jumped out at me.

tsconfig.json

package.json

src/send-message.ts

timabbott · 2022-11-28T21:02:07Z

.eslintrc.json

+    "@typescript-eslint/no-floating-promises": "off",
+    "unicorn/prefer-top-level-await": "off"
+  }
+}


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

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)

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

timabbott · 2022-11-28T21:06:19Z

@Mogztter it'd be great for you to review/test this as well when you get a chance. (@klardotsh for context, he's the original author).

klardotsh · 2022-11-30T01:18:05Z

I've done the documentation updates I wanted to do and have a successful simple regression test in https://github.com/klardotsh/zulip-gha-playground/actions/runs/3579426796, so I'm marking this ready for review!

send-message/README.md

alexmv

I'm not qualified to review the actual Typescript changes (paging @andersk if he has any ideas about cleaning up the AsyncResultWrapper / wrapAsync in getZulipClient), but the rest of this looks great to me.

alexmv · 2022-11-30T16:36:21Z

src/send-message.ts

+    Result.all(
+      getMandatoryInputFromJob("content"),
+      getDestinationDetails(),
+      await getZulipClient()
+    )


I assume that using [AsyncResult.all](https://github.com/movesthatmatter/ts-async-results#combining-results) didn't bear any fruit?

alexmv · 2022-12-05T23:54:25Z

Not having heard any objections, and since this is a clear improvement on the current state of affairs, I've merged this -- thanks for all of the cleanups, @klardotsh!

klardotsh requested review from andersk, alexmv and timabbott November 24, 2022 04:02

klardotsh commented Nov 24, 2022

View reviewed changes

alexmv reviewed Nov 28, 2022

View reviewed changes

tsconfig.json Show resolved Hide resolved

package.json Show resolved Hide resolved

src/send-message.ts Outdated Show resolved Hide resolved

src/send-message.ts Outdated Show resolved Hide resolved

timabbott reviewed Nov 28, 2022

View reviewed changes

klardotsh added 6 commits November 28, 2022 14:58

deps: add and configure typescript

3014fe3

deps: Install and configure ESLint based on XO defaults.

ecdacd5

types: Provide basic structural type hints for zulip-js.

c09b4a0

send-message: Port action to strict TypeScript.

1d7707f

markdown: Lint and fix with prettier.

79c1ebc

docs: Add contribution guidelines based on zulip/zulip.

4cfd4b7

klardotsh marked this pull request as ready for review November 30, 2022 01:18

alexmv reviewed Nov 30, 2022

View reviewed changes

send-message/README.md Outdated Show resolved Hide resolved

klardotsh added 3 commits November 29, 2022 20:14

docs: Reformat send-message documentation into a condensed table.

aac372e

docs: Document a bot-specific note about which email address to use.

dc7e6c7

docs: Clarify required state of topic parameter.

7dadd3d

klardotsh mentioned this pull request Nov 30, 2022

Bump Node v12 to v16 #71

Merged

alexmv reviewed Nov 30, 2022

View reviewed changes

klardotsh mentioned this pull request Dec 1, 2022

tools: Reimplement CI failure message script to not call curl itself. zulip/zulip#23719

Merged

12 tasks

alexmv merged commit 28fa5c6 into zulip:main Dec 5, 2022

klardotsh deleted the typescriptification branch December 6, 2022 21:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Maintainability Items: linting and typing #69

Maintainability Items: linting and typing #69

klardotsh commented Nov 24, 2022 •

edited

Loading

klardotsh Nov 24, 2022

klardotsh Nov 24, 2022

klardotsh Nov 24, 2022

alexmv Nov 30, 2022

alexmv left a comment

timabbott Nov 28, 2022

klardotsh Nov 28, 2022

klardotsh Nov 28, 2022

timabbott commented Nov 28, 2022

klardotsh commented Nov 30, 2022

alexmv left a comment

alexmv Nov 30, 2022

alexmv commented Dec 5, 2022

Maintainability Items: linting and typing #69

Maintainability Items: linting and typing #69

Conversation

klardotsh commented Nov 24, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

timabbott commented Nov 28, 2022

klardotsh commented Nov 30, 2022

alexmv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexmv commented Dec 5, 2022

klardotsh commented Nov 24, 2022 •

edited

Loading