convert Focus to TypeScript #700
Conversation
|
IMO the |
| return this.supportedCommands.indexOf(cmd) !== -1; | ||
| } | ||
|
|
||
| async _write_parts(parts, cb) { | ||
| async _write_parts(parts: any[], cb: any) { |
There was a problem hiding this comment.
This method is not used anywhere, remove?
| if (this[methodName]) { | ||
| const tmp = this[methodName]; | ||
| this[methodName] = (...args) => { | ||
| addMethod(methodName: string, command: string) { |
There was a problem hiding this comment.
This method while being used in two places,
the added methods are not being used.
I think this can be removed.
|
Hi @alexpargon, I notice that you migrated some code into I take a look at the new code. IMO one concern about the new code is its testability. You can consider applying Clean Architecture or Hexagonal Architecture |
This allows quick check of typescript error without running `yarn make-dev` which is much slower
The conversion will be done in many small PRs, so that the changes can be merged quickly, minimizing merge conflicts for the team.
The name does not accurately match behavior. Suggest fixing it in the future
`w` is the convention I use as I'm lazy. Feel free to suggest otherwise
the `num2hexstr` usage actually shows that the first param is a string, and the second param is opional based on the commented console log. the value is defaults to 0 to better match its intent and passes type check.
to enable further refactoring
that is more inline to the behavior
They are not used elsewhere
|
Hey there @unional ! You are right about the new comms block, but it's a change that was required as the new methods of communication require further integration with the Node engine (HID), I have just implemented this feature as is but will better abstract it as soon as we release the Bluetooth capable Bazecor (HID), that will be my next priority because testing is a must and after your efforts, it's getting simpler to write good tests. |
|
Sounds good. Let me know if there is any area you need help to clean up / redesign / add tests. I'm even thinking of rewriting kaleidoscope in Rust or Zig. 🤣 |
This branch is based on
ts1thus includes all changes in that branch.This can be merged after #655 is merged.
I am opening this PR now to gather feedbacks.