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.

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

Fix nullable input/output with type converters #130

Merged
merged 8 commits into from
Oct 6, 2023
Merged

Fix nullable input/output with type converters #130

merged 8 commits into from
Oct 6, 2023

Conversation

ekerik220
Copy link
Contributor

No description provided.

@ekerik220 ekerik220 mentioned this pull request Oct 5, 2023
Closed
@tenhobi tenhobi linked an issue Oct 5, 2023 that may be closed by this pull request
Nullable input/output with type converters #128
Closed
@tenhobi
Copy link
Member

tenhobi commented Oct 5, 2023

Great, so tests are the only thing left, right? @ekerik220 Will you add them? ☺️

@ekerik220
Copy link
Contributor Author

@tenhobi That's right! I'll try add them today. 👍

@ekerik220 ekerik220 marked this pull request as ready for review October 6, 2023 01:47
@ekerik220
Copy link
Contributor Author

@tenhobi I've added the tests! Please take a look at them when you have time and let me know if I should change/add anything.

@ekerik220 ekerik220 requested a review from tenhobi October 6, 2023 01:51
@tenhobi
Copy link
Member

tenhobi commented Oct 6, 2023 •
edited
Loading

@ekerik220 looks good, but right now when we have a nullable source, we always test it with a value. Nullable sources should have two tests - w/ data and w/o data (there null is passed). 😊

@ekerik220
Copy link
Contributor Author

@tenhobi Good point! I've added the null input tests.

@ekerik220 ekerik220 requested a review from tenhobi October 6, 2023 07:47
@ekerik220 ekerik220 changed the title [wip] Fix nullable input/output with type converters Fix nullable input/output with type converters Oct 6, 2023
tenhobi
tenhobi previously approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@tenhobi tenhobi left a comment

Choose a reason for hiding this comment

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

LGTM, thanks a lot!

tenhobi
tenhobi approved these changes Oct 6, 2023
View reviewed changes
Copy link
Member

@tenhobi tenhobi left a comment

Choose a reason for hiding this comment

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

LGTM, just wanted to make DCM work for forks, but will do it later.

@tenhobi tenhobi merged commit 892a27d into netglade:main Oct 6, 2023
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tenhobi tenhobi tenhobi approved these changes

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

Successfully merging this pull request may close these issues.

Nullable input/output with type converters
2 participants
@ekerik220 @tenhobi