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

Refactor core type generics naming #592

Closed
wants to merge 1 commit into from

Conversation

baileytincher
Copy link
Contributor

@baileytincher baileytincher commented Jan 3, 2021

Somewhat a WIP still. It would be good to come up with a way to test type definitions more thoroughly though. One option is to move some (or all) of the test files to TypeScript and that will naturally catch some potential breaking changes in future PRs.

I found the use of the EventType and HandlerReturnType definitions to be confusing as I was working on this. I think they can be simplified or removed entirely, but I'm going to mull over their purpose a bit. Would appreciate input on what they are meant to do as I did not find a need for such a construct in working on my own framework https://github.com/ShallotJS/shallot/blob/4781b6700e637d1e15dedf99d5202ddd799888b8/src/aws/index.ts#L44-L52

What does this implement/fix? Explain your changes.

Does this close any currently open issues?

#585 (comment)

Todo list

[ ] Feature/Fix fully implemented
[ ] Added tests
[ ] Updated relevant documentation
[ ] Updated relevant examples

@lmammino
Copy link
Member

lmammino commented Jan 3, 2021

Great contribution @baileytincher! Thanks a lot.

How would you suggest to test these changes?

Also, be aware that the CI is currently failing the linting step.

@willfarrell
Copy link
Member

willfarrell commented Jan 3, 2021

I think the linting error was from some earlier incomplete code I pushed. It was fixed last night. Re-pull, to resolve it.

@willfarrell willfarrell marked this pull request as draft January 4, 2021 07:18
@willfarrell
Copy link
Member

Closing in favour of #616

@willfarrell willfarrell closed this Mar 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants