Skip to content

Add more comments #1993

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 28 commits into
base: main
Choose a base branch
from
Open

Add more comments #1993

wants to merge 28 commits into from
+65,534 −22,978

Conversation

Bashamega
Copy link
Contributor

related to #1940
Hello:)
In my last PR I have used MDN to generate comments for interface declarations, in this pr I add it for the properties.

@github-actions GitHub Actions
Copy link
Contributor

Thanks for the PR!

This section of the codebase is owned by @saschanaz - if they write a comment saying "LGTM" then it will be merged.

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025 •
edited
Loading

Not ready for review
Ready for review

saschanaz
saschanaz reviewed Apr 23, 2025
View reviewed changes
Copy link
Contributor

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

I'm yet to review this, but please do not convert to sync APIs.

@Bashamega
Copy link
Contributor Author

I'm yet to review this, but please do not convert to sync APIs.

Sure

@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 23, 2025 •
edited
Loading

Hello @saschanaz
I have switched the old functions to async, but the new function is still sync because the emitter is not async. I tried switching it to async, but it broke.

saschanaz
saschanaz reviewed Apr 24, 2025
View reviewed changes
@Bashamega
Copy link
Contributor Author

I have updated the context to not call the API in the emitter, but that cause the test to fail, I will fix it later

@Bashamega
Copy link
Contributor Author

Done @saschanaz

saschanaz
saschanaz reviewed Apr 24, 2025
View reviewed changes
This was referenced Apr 25, 2025
Open
Open
@Bashamega
Copy link
Contributor Author

Bashamega commented Apr 26, 2025 •
edited
Loading

Hello @saschanaz, @jakebailey
Do you have any other change requests in this PR?

@Bashamega
Copy link
Contributor Author

Hello @saschanaz
I have updated the emitter but I think the formatted is better to be a separate issue

@Bashamega
Copy link
Contributor Author

I have updated the script to use promise @saschanaz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@saschanaz saschanaz saschanaz left review comments

@jakebailey jakebailey Awaiting requested review from jakebailey

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Bashamega @saschanaz @jakebailey