Add Term Type #361

j285he · 2021-11-16T21:42:58-05:00

j285he commented

2021-11-16 21:42:58 -05:00

No description provided.

j285he added 2 commits 2021-11-16 21:42:59 -05:00

continuous-integration/drone/push Build is failing Details

e5fd9bfec9 Add Term type

continuous-integration/drone/push Build is failing Details

131a667ddf TermEvent->TermPage for consistency

j285he added 1 commit 2021-11-16 21:45:23 -05:00

continuous-integration/drone/push Build is failing Details

691b749bb7 Add type to getAllEvents

j285he commented

2021-11-16 21:51:05 -05:00

I think problem with generate-calendar not working is that it imports getAllEvents which is in the file lib/events.ts that imports the new type from @/utils. Since generate-calendar.ts is a ts-node file the paths/aliases in tsconfig don't work (https://github.com/TypeStrong/ts-node/issues/138)

Some solutions may be

use a library like this https://www.npmjs.com/package/tsconfig-paths
in events.ts, use ../utils instead of @/utils -- we could do this for all lib files to make things consistent

Or may be I'm missing something. What do you think?

I think problem with `generate-calendar` not working is that it imports `getAllEvents` which is in the file `lib/events.ts` that imports the new type from `@/utils`. Since `generate-calendar.ts` is a `ts-node` file the paths/aliases in `tsconfig` don't work (https://github.com/TypeStrong/ts-node/issues/138) Some solutions may be * use a library like this https://www.npmjs.com/package/tsconfig-paths * in `events.ts`, use `../utils` instead of `@/utils` -- we could do this for all lib files to make things consistent Or may be I'm missing something. What do you think?

j285he requested review from n3parikh 2021-11-16 21:51:25 -05:00

j285he requested review from a258wang 2021-11-20 19:51:51 -05:00

j285he added 1 commit 2021-11-20 21:12:20 -05:00

continuous-integration/drone/push Build is passing Details

fd5c244f98 Change alias for utils

a258wang reviewed 2021-11-20 22:37:05 -05:00

utils.ts

													
				@ -1,3 +1,11 @@

				export const TERMS = ["winter", "spring", "fall"] as const;

				export type Term = typeof TERMS[number];

a258wang commented

2021-11-20 22:37:05 -05:00

As a noob, I'm curious: what's the advantage to defining the Term type this way, instead of using a union of strings ("winter" | "spring" | "fall")?

As a noob, I'm curious: what's the advantage to defining the `Term` type this way, instead of using a union of strings (`"winter" | "spring" | "fall"`)?

j285he commented

2021-11-21 15:01:38 -05:00

I saw other files (lib/events and lib/news) specifically defining TERMS = ["winter", "spring", "fall"] and I thought it would be useful to move that into utils. More generally, if we were to change what TERMS is, say if "spring" was renamed to "summer", then we would only need one code change here (although that's probably never going to happen).

Term = typeof TERMS[number] is the syntax to transform the const array to type "winter" | "spring" | "fall".

I saw other files (`lib/events` and `lib/news`) specifically defining `TERMS = ["winter", "spring", "fall"]` and I thought it would be useful to move that into `utils`. More generally, if we were to change what `TERMS` is, say if "spring" was renamed to "summer", then we would only need one code change here (although that's probably never going to happen). `Term = typeof TERMS[number]` is the syntax to transform the const array to type `"winter" | "spring" | "fall"`.

👍 1

a258wang reviewed 2021-11-20 22:39:02 -05:00

utils.ts Outdated

													
				@ -1,0 +3,4 @@

				// https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates

				export function isTerm(x: string): x is Term {

				  return x === "winter" || x === "spring" || x === "fall";

a258wang commented

2021-11-20 22:39:02 -05:00

Is there a reason for this approach instead of return TERMS.includes(x);?

Is there a reason for this approach instead of `return TERMS.includes(x);`?

j285he commented

2021-11-21 14:58:49 -05:00

Because I used as const on TERMS, I believe TypeScript infers the "narrowest type", so TypeScript identifies TERMS as a readonly array of ["winter", "spring", "fall"] instead of an array of strings. Therefore using return TERMS.includes(x) gives me the error Argument of type 'string' is not assignable to parameter of type '"winter" | "spring" | "fall"'.

I think the reasoning for using as const to get the "narrowest type" is so that we can define the type Term using the array TERMS. If as const was not present then Term would just be type string[] (which is not specific enough for our purposes).

Because I used `as const` on `TERMS`, I believe TypeScript infers the "narrowest type", so TypeScript identifies `TERMS` as a readonly array of `["winter", "spring", "fall"]` instead of an array of strings. Therefore using `return TERMS.includes(x)` gives me the error `Argument of type 'string' is not assignable to parameter of type '"winter" | "spring" | "fall"'`. I think the reasoning for using `as const` to get the "narrowest type" is so that we can define the type `Term` using the array `TERMS`. If `as const` was not present then `Term` would just be type `string[]` (which is not specific enough for our purposes).

j285he commented

2021-11-21 15:02:22 -05:00

Let me know if there's a way to improve this. I agree it does look clunky

a258wang commented

2021-11-21 23:07:44 -05:00

Ahh okay that's really interesting, thanks for the explanation!

Personally, I think the way this is written right now is actually quite readable. My only concern was if we ever wanted to, say, change "spring" to "summer", then we would have to make the change in two separate places (albeit two places in the same file).

I just did some experimenting and it seems like return TERMS.some(term => x === term); could work - this solves the "change in two separate places" issue, but it's also a tad less readable in my opinion. I'll leave it up to you to decide what to use!

Ahh okay that's really interesting, thanks for the explanation! Personally, I think the way this is written right now is actually quite readable. My only concern was if we ever wanted to, say, change "spring" to "summer", then we would have to make the change in two separate places (albeit two places in the same file). I just did some experimenting and it seems like `return TERMS.some(term => x === term);` could work - this solves the "change in two separate places" issue, but it's also a tad less readable in my opinion. I'll leave it up to you to decide what to use!

j285he commented

2021-11-22 13:15:52 -05:00

Good point and good idea. Wonder if anyone else has thoughts or opinions on this.

j285he commented

2021-11-22 17:10:59 -05:00

I liked the implementation and changed it. Let me know if anyone wants it reversed.

👍 1

a258wang approved these changes 2021-11-21 23:10:33 -05:00

j285he added 2 commits 2021-11-22 13:14:36 -05:00

9e0aec388c Merge branch 'main' into j285he-add-term-type

continuous-integration/drone/push Build is passing Details

73cf9530be Add Term type to members

j285he added 1 commit 2021-11-22 17:10:25 -05:00

continuous-integration/drone/push Build is passing Details

92a115fc35 Use TERMS.some

j285he added 1 commit 2021-11-23 12:56:58 -05:00

continuous-integration/drone/push Build is passing Details

5e925e3e59 Fix members type error

j285he added 1 commit 2021-11-30 18:35:42 -05:00

a5236ef033 Test CI for type errors

j285he added 1 commit 2022-01-11 18:35:28 -05:00

continuous-integration/drone/push Build is failing Details

5a87174b13 Merge branch 'main' into j285he-add-term-type

j285he added 1 commit 2022-01-11 18:47:30 -05:00

continuous-integration/drone/push Build is passing Details

95287b4de7 Fix type mistake (?)

j285he commented

2022-01-11 18:58:39 -05:00

Our CI does highlight type errors with the new custom Term type, see 5a87174b13 vs 95287b4de7.

Our CI *does* highlight type errors with the new custom Term type, see 5a87174b13bcaa6857c43c0a79cfe7a87a55d4f8 vs 95287b4de7aa4e0e3175d8865015efc4ff83d977.

👍 2

n3parikh approved these changes 2022-01-19 17:42:18 -05:00

n3parikh left a comment

Looks great, thanks for this!

Sorry for the very slow review

Looks great, thanks for this! Sorry for the very slow review

lib/members.ts

													
				@ -12,4 +11,0 @@

				): Promise<Member[]> {

				  if (term !== "winter" && term !== "spring" && term !== "fall") {

				    throw new Error(`[getMembers] Not a valid term: "${term}"`);

				  }