Skip to content
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

Convert from JavaScript to TypeScript #66

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open

Conversation

Delaney
Copy link

@Delaney Delaney commented Sep 4, 2024

Addressing #9

@angelikatyborska
Copy link
Member

Oh, awesome! Thank you so much for all that work 💙 I should have some time on the weekend to review this PR in detail.

Copy link
Member

@angelikatyborska angelikatyborska left a comment

Choose a reason for hiding this comment

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

I left a whole bunch of comments with feedback. I think there is some chaos in the type definitions and how they're used, and there's too much usage of as ... in the source code (not it tests, in tests in takes sense).

If you have more time to work on this PR, I would be happy to receive changes for my feedback, but if you don't have time anymore, I can also make those changes myself. Let me know!

I didn't test yet if the library still works after the changes. I would only do that later, at the end of the review process when I expect no more changes to be made to the source code (so that I don't have to test this more than once, it's a slow and manual process).

types/global.type.ts Outdated Show resolved Hide resolved
types/global.type.ts Outdated Show resolved Hide resolved
types/global.type.ts Outdated Show resolved Hide resolved
lib/comment.ts Outdated Show resolved Hide resolved
lib/comment.ts Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
.eslintrc.json Outdated Show resolved Hide resolved
@Delaney
Copy link
Author

Delaney commented Sep 8, 2024

I left a whole bunch of comments with feedback. I think there is some chaos in the type definitions and how they're used, and there's too much usage of as ... in the source code (not it tests, in tests in takes sense).

If you have more time to work on this PR, I would be happy to receive changes for my feedback, but if you don't have time anymore, I can also make those changes myself. Let me know!

I didn't test yet if the library still works after the changes. I would only do that later, at the end of the review process when I expect no more changes to be made to the source code (so that I don't have to test this more than once, it's a slow and manual process).

Thanks for the feedback. I'll work on your comments in the next few days and revert.

@Delaney
Copy link
Author

Delaney commented Oct 4, 2024

Hi @angelikatyborska 👋🏾 ready for another review

@angelikatyborska
Copy link
Member

Thank you for the changes 🙏 I currently don't have time for open-source work, but this should change in a few weeks. Your contributions are appreciated and I WILL review them, just not right now. Sorry about that!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants