Biome1.5.1 #5867
Biome1.5.1 #5867
Conversation
chriswk
requested review from
daveleek,
kwasniew,
nunogois,
thomasheartman and
sighphyre
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
1 Ignored Deployment
|
| @@ -26,7 +26,7 @@ jest.mock('@slack/web-api', () => ({ | |||
| })); | |||
|
|
|||
| describe('SlackAppAddon', () => { | |||
| let addon; | |||
| let addon: SlackAppAddon; | |||
There was a problem hiding this comment.
Shouldn't the type be | undefined? because you're not assigning it a value on declaration
| await addon.handleEvent(eventWith2Tags, { accessToken }); | ||
| await addon.handleEvent(eventWith2Tags, { | ||
| accessToken, | ||
| defaultChannels: '', |
There was a problem hiding this comment.
I guess default channels should be allowed to be undefined... maybe the type definition is wrong?
There was a problem hiding this comment.
This also works, since defaultChannels must be truthy for us to treat it as set, and the empty string is falsy.
But yes, future improvement could be to make defaultChannels optional.
| @@ -102,6 +102,7 @@ class StateController extends Controller { | |||
| const userName = extractUsername(req); | |||
| const { drop, keep } = req.query; | |||
| // TODO: Should override request type so file is a type on request | |||
| // biome-ignore lint/suspicious/noImplicitAnyLet: <explanation> | |||
There was a problem hiding this comment.
This is prod code, maybe we can add an explanation, or just don't know why this is ignored rather than
| environment: '', | ||
| projects: [], | ||
| tokenName: '', |
There was a problem hiding this comment.
I guess this is fine, but unsure if this ends up being the same as sending undefined. I trust it because the test fails, but I'm not sure if we're actually testing the output of this insert
| @@ -125,15 +132,18 @@ test('Merge keeps value for single row in database', async () => { | |||
| appName: clientRegistration.appName, | |||
| description: 'new description', | |||
| }); | |||
| const stored = await clientApplicationsStore.getApplication( | |||
| const stored = await clientApplicationsStore.get( | |||
There was a problem hiding this comment.
getApplication and get methods are the same, should we remove one of them?
There was a problem hiding this comment.
getApplication doesn't exist on the store interface. it was written only on the fake store, but yes, we could remove it.
There was a problem hiding this comment.
unleash/src/lib/db/client-applications-store.ts
Lines 157 to 169 in e27a578
Co-authored-by: Gastón Fournier <[email protected]>
Lots of work here, mostly because I didn't want to turn off the
noImplicitAnyLetlint. This PR tries its best to type all the untyped lets biome complained about (Don't ask me how many hours that took or how many lints that was >200...), which in the future will force test authors to actually type their global variables setup inbeforeAll.