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

feat: verb request and response types can be any FTL type #1912

Merged
merged 2 commits into from
Jul 15, 2024

Conversation

matt2e
Copy link
Collaborator

@matt2e matt2e commented Jul 1, 2024

closes #1854

@matt2e matt2e requested a review from alecthomas as a code owner July 1, 2024 00:05
@matt2e matt2e requested review from a team and worstell and removed request for a team July 1, 2024 00:05
@ftl-robot ftl-robot mentioned this pull request Jul 1, 2024
@matt2e
Copy link
Collaborator Author

matt2e commented Jul 1, 2024

I realised afterwards that the issue refers to only making the response type more lenient, not the request type. Lmk if I should keep the request type strict

if err != nil {
return fmt.Errorf("HTTP request body is not valid JSON: %w", err)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's odd to me that these checks are in the ingress package even though they check the validity of request data for all requests.
I've removed "HTTP" from the error message because this is being used outside of HTTP use cases

Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't make much sense for this function to be in this package anymore really.

@matt2e matt2e force-pushed the matt2e/non-struct-req-resp branch from dff622c to 6da5eea Compare July 1, 2024 00:09
Copy link
Collaborator

@alecthomas alecthomas left a comment

Choose a reason for hiding this comment

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

Can you add an integration test for this please?

if err != nil {
return fmt.Errorf("HTTP request body is not valid JSON: %w", err)
Copy link
Collaborator

Choose a reason for hiding this comment

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

It doesn't make much sense for this function to be in this package anymore really.

@alecthomas
Copy link
Collaborator

Request or response type is fine, let's merge.

@matt2e matt2e force-pushed the matt2e/non-struct-req-resp branch from 6da5eea to c71696d Compare July 10, 2024 01:53
@wesbillman
Copy link
Collaborator

Should we merge this one @matt2e or are we holding off for stability?

# Conflicts:
#	backend/controller/ingress/ingress.go
@matt2e matt2e force-pushed the matt2e/non-struct-req-resp branch from c71696d to 0e30991 Compare July 15, 2024 02:08
@matt2e matt2e force-pushed the matt2e/non-struct-req-resp branch from 7f0c29a to 4b103b3 Compare July 15, 2024 03:32
@matt2e matt2e merged commit 1f073ab into main Jul 15, 2024
55 checks passed
@matt2e matt2e deleted the matt2e/non-struct-req-resp branch July 15, 2024 05:32
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.

Allow verbs to return arbitrary types
4 participants