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

[dio] Test typed responses #1755

Merged
merged 8 commits into from
Dec 28, 2023
Merged

Conversation

kuhnroyal
Copy link
Member

@kuhnroyal kuhnroyal commented Mar 22, 2023

This PR relates to #1335 and is for now just meant as idea and for further testing.
But correctly typed responses do work.

New Pull Request Checklist

  • I have read the Documentation
  • I have searched for a similar pull request in the project and found none
  • I have updated this branch with the latest main branch to avoid conflicts (via merge from master or rebase)
  • I have added the required tests to prove the fix/feature I'm adding
  • I have updated the documentation (if necessary)
  • I have run the tests without failures
  • I have updated the CHANGELOG.md in the corresponding package

Additional context and info (if any)

@kuhnroyal kuhnroyal added s: feature This issue indicates a feature request p: dio Targeting `dio` package labels Mar 22, 2023
@kuhnroyal kuhnroyal self-assigned this Mar 22, 2023
@kuhnroyal kuhnroyal force-pushed the feature/response-type branch 2 times, most recently from 2c5e5fe to fa39db6 Compare March 22, 2023 14:33
@AlexV525 AlexV525 changed the title Test typed responses [dio] Test typed responses Mar 27, 2023
@kuhnroyal kuhnroyal force-pushed the feature/response-type branch from fa39db6 to 72fe7d2 Compare May 15, 2023 20:17
@kuhnroyal kuhnroyal changed the base branch from main to 6.0.0 May 15, 2023 20:19
@kuhnroyal
Copy link
Member Author

@AlexV525 @ueman I had this open for a while and would like some feedback for 6.0.0 :)

@kuhnroyal kuhnroyal force-pushed the feature/response-type branch from 72fe7d2 to 3f9e941 Compare June 1, 2023 21:15
@kuhnroyal
Copy link
Member Author

Really would love some feedback here :)

Copy link
Contributor

@ueman ueman left a comment

Choose a reason for hiding this comment

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

I like it, good work

Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

This is absolutely a significant step forward for the package. So everyone should write request<String?> instead of request<String>, right? If they upgraded without thinking, this might produce much NPE during the runtime?

@AlexV525
Copy link
Member

The change is great, but we might evaluate the cost for the whole ecosystem.

@kuhnroyal
Copy link
Member Author

kuhnroyal commented Sep 11, 2023

I agree that this can break. So we need good examples in the 6.0.0 migration docs.

But users who have handling for nullable responses via response?.data or if (response.data != null) will receive compiler hints/warnings at these points and need to figure out what they want or expect. Good samples are the key here.

@kuhnroyal
Copy link
Member Author

I added a changelog and some basic samples to the migration docs.
Really think we should do this. We have to do it at some point, might as well get it over with now :D

@kuhnroyal kuhnroyal marked this pull request as ready for review December 21, 2023 22:24
@kuhnroyal kuhnroyal requested a review from a team as a code owner December 21, 2023 22:24
Copy link
Member

@AlexV525 AlexV525 left a comment

Choose a reason for hiding this comment

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

🚀

@kuhnroyal kuhnroyal merged commit 88a0fda into cfug:6.0.0 Dec 28, 2023
3 checks passed
@kuhnroyal kuhnroyal deleted the feature/response-type branch December 28, 2023 13:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
p: dio Targeting `dio` package s: feature This issue indicates a feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants