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

fix: propogate type through Provider #744

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

philipbjorge
Copy link

Before
CleanShot 2023-09-08 at 14 46 06

After
CleanShot 2023-09-08 at 14 45 45

@jonathangreen
Copy link

@rmk135 this seems like a big improvement for type safety, any change of this getting merged in?

@rmk135 rmk135 requested review from rmk135 and ZipFile January 7, 2025 00:25
@rmk135
Copy link
Member

rmk135 commented Jan 7, 2025

@philipbjorge thank you for the contribution. This change looks good to me. I'm not sure why there was no generic propagation. Looks like a good bugfix.

There also should be some tests for that. Maybe we will need to fix them. Not sure why CI is not running checks for this PR.

@rmk135 rmk135 changed the base branch from master to develop January 7, 2025 00:27
@rmk135
Copy link
Member

rmk135 commented Jan 7, 2025

@ZipFile can you please review this PR as well?

@philipbjorge
Copy link
Author

I don't have capacity to address any feedback here and no longer work at the company using this library -- But I've reached out to my previous teammates to see if they can carry the torch :)

Thanks for reviewing! Excited to see this project waking back up!

Copy link
Contributor

@ZipFile ZipFile left a comment

Choose a reason for hiding this comment

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

Looks good to me either, but it would be nice to have some tests too.

@coveralls
Copy link

Coverage Status

coverage: 93.745%. remained the same
when pulling 579081b on pachama:type-provider
into 46646b1 on ets-labs:develop.

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.

6 participants