Skip to content
This repository was archived by the owner on Nov 15, 2024. It is now read-only.

feat minut noise thresholds list#181

Merged
itelo merged 18 commits into
mainfrom
feat-minut-noise-thresholds-list
Apr 18, 2023
Merged

feat minut noise thresholds list#181
itelo merged 18 commits into
mainfrom
feat-minut-noise-thresholds-list

Conversation

@itelo

@itelo itelo commented Apr 18, 2023

Copy link
Copy Markdown
Contributor
  • feat: add new method to list noise thresholds
  • add noise thresholds list
  • fix variable NOISE_DETECTION_DEVICE_TYPES
  • debug ci
  • maybe the error is in minut scenario
  • more debug
  • even more debug
  • retry
  • missing await
  • final try
  • remove logs
  • another try
  • add serial to test
  • add serial
  • remove serial add modifiesState
  • remove ts-ignore from startandseedserver

@itelo itelo force-pushed the feat-minut-noise-thresholds-list branch from fac2be2 to af387a0 Compare April 18, 2023 15:02
@itelo itelo marked this pull request as ready for review April 18, 2023 15:02
@itelo itelo requested a review from codetheweb as a code owner April 18, 2023 15:02
@itelo itelo enabled auto-merge (squash) April 18, 2023 15:12
Comment on lines +98 to +113
const devices = {
...load_devices_from.reduce((acc, provider, index) => {
if (provider === "schlage") {
acc["schlageLock"] = devicesByProvider[index]
return acc
}

if (provider === "august") {
acc["augustLock"] = devicesByProvider[index]
return acc
}

acc[provider] = devicesByProvider[index]
return acc
}, {} as Record<"augustLock" | "minut" | "schlageLock", any>),
}

@seveibar seveibar Apr 18, 2023

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The readability here is hard, here's why:

  • There's an indexing scheme that's not clear, particularly because some of the variable names are unclear. I would consider renaming devicesByProvider and load_devices_from (poor name more generally, it is a verb which implies it's a function)
  • The type is buried in the implementation, if something starts getting complicated, better to lift the type up to the top
  • .reduce isn't a great function because it moves the type definition towards the bottom, in practice loops tend to be preferable
  • Mark deprecated or legacy behavior explicitly
  • I THINK there's a spread here for no reason? Why spread the reduce

Principally, we should try to avoid letting optimizations like Promise.all impact readability.

Suggested change
const devices = {
...load_devices_from.reduce((acc, provider, index) => {
if (provider === "schlage") {
acc["schlageLock"] = devicesByProvider[index]
return acc
}
if (provider === "august") {
acc["augustLock"] = devicesByProvider[index]
return acc
}
acc[provider] = devicesByProvider[index]
return acc
}, {} as Record<"augustLock" | "minut" | "schlageLock", any>),
}
const devices: Record<keyof providersMap & 'schlageLock' | 'augustLock', any> = {}
for (let i = 0; i < load_devices_from.length; i++) {
const provider = load_devices_from[i]
devices[provider] = devicesByProvider[i]
}
// TODO: remove when we deprecate schlageLock and augustLock
if (devices.august) {
devices.augustLock = devices.august
}
if (devices.schlage) {
devices.schlageLock = devices.schlage
}

@itelo itelo requested a review from seveibar April 18, 2023 17:06
@itelo itelo merged commit 5881526 into main Apr 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants