-
-
Notifications
You must be signed in to change notification settings - Fork 7.5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
types: Migrated todo_widget.js to todo_widget.ts. #29243
base: main
Are you sure you want to change the base?
Conversation
df063b1
to
775c46d
Compare
This feels very hard to review;
These should largely be done via adding You also seem to have re-sorted the functions. Please don't do that in a TS migration commit; if you need to do it, do it in an easily reviewed function either before or after. @Wr4th100 can you rework this to be more reviewable? |
775c46d
to
63d95f7
Compare
I changed the order, because eslint was complaining about it (
Sure, I have made the changes now, please have a look! |
3f972aa
to
63d95f7
Compare
72c09a7
to
29ca4ba
Compare
5c383cc
to
5de10f4
Compare
@timabbott, I have made the necessary changes, and all the checks have passed, please have a look! |
@Wr4th100 Can you provide an update on this migration? This surely requires a rebase if anything. It would be nice to get this migrated! |
5de10f4
to
e5d50d9
Compare
@@ -39,59 +114,59 @@ export class TaskData { | |||
} | |||
|
|||
for (const [i, data] of tasks.entries()) { | |||
this.handle.new_task.inbound("canned", { | |||
this.handle.new_task.inbound(message_sender_id, { | |||
key: i, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Normally, the inbound
function receives the sender_id
argument as number. But here, they have mentioned canned
here. I have changed that to message_sender_id
.
Is this correct? Or should I be changing something else?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change the type annotations of the parameter of the inbound
function to accept string | number
, otherwise, it might result in a key error. You can read the CZO thread for more context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, when making these types of changes, it's recommended to create a separate commit and provide a valid reason for why you chose that approach.
466ce32
to
6df1fa3
Compare
Hey @Wr4th100, are you looking forward to completing this one? |
6df1fa3
to
7cc7be5
Compare
@Wr4th100 can you comment on how you resolved the feedback? I guess @N-Shar-ma should probably review this in any case. |
Yeah sure! As @afeefuddin mentioned, I have changed the type of the parameter of the |
extra_data: { | ||
task_list_title?: string; | ||
tasks?: {task: string; desc: string}[]; | ||
} | null; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type for this has already been defined in widgetize.ts, which you can move to this file now.
In general, try to base this work off poll_widget.ts
since these are pretty similar files, and ensure to test thoroughly by sending and interacting with a variety of poll messages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The type for this has already been defined in widgetize.ts, which you can move to this file now.
I tried using the type, but it didn't work out quite well. It resulted in a lot of errors.
In general, try to base this work off
poll_widget.ts
since these are pretty similar files, and ensure to test thoroughly by sending and interacting with a variety of poll messages.
Yes, I have based this migration on poll_widget.ts
. Also, I have tested the Todo Widget extensively, by trying all the different ways the to-do widget can be interacted with.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried using the type, but it didn't work out quite well. It resulted in a lot of errors.
Could you be more specific about the errors? If the type that had been working so far is resulting in errors, it's highly likely the problem is with the rest of the code.
Migrated todo_widget.js to todo_widget.ts for enhanced type-safety. Changed the file name in tools/test-js-with-node.
b032164
to
55d2dc9
Compare
todo_widget.js
to Typescript for improved type safety and maintainability.tests-js-with-node
.Self-review checklist
(variable names, code reuse, readability, etc.).
Communicate decisions, questions, and potential concerns.
Individual commits are ready for review (see commit discipline).
Completed manual review and testing of the following: