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: add required to responseField tag and append the required fields to generateResponseContentSpec for object types #814

Merged
merged 11 commits into from
Oct 18, 2024

Conversation

yannick-softwerft
Copy link
Contributor

@yannick-softwerft yannick-softwerft commented Feb 23, 2024

Extends the documentation for @responseField tags by adding the required field.

Format before was:

// Format:
// @responseField <name> <type>  <description>

Format after is:

// Format:
// @responseField <name> <type> <"required" (optional)> <description>

If required is set in the documentation it will set the new $required- property (ResponseField Dto) to true.
All fields that have the required property set to true will be added to the openapi spec for object types.

@shalvah
Copy link
Contributor

shalvah commented Feb 27, 2024

Could you add some tests as well? You don't necessarily need to add new tests, just find some of the tests around @responseField and #[ResponseField] and ensure they generate the correct parameter values. Also add some cases in the OpenAPI spec writer test that handle required/optional.

@yannick-softwerft
Copy link
Contributor Author

Could you add some tests as well? You don't necessarily need to add new tests, just find some of the tests around @responseField and #[ResponseField] and ensure they generate the correct parameter values. Also add some cases in the OpenAPI spec writer test that handle required/optional.

Yes, I will do that, but I will probably not be able to do it until 15.03.2024.

@yannick-softwerft yannick-softwerft marked this pull request as draft March 1, 2024 10:58
@shalvah
Copy link
Contributor

shalvah commented Mar 15, 2024

Ready for review? It's still marked as draft.

@sotdan
Copy link
Contributor

sotdan commented Sep 26, 2024

Hey @shalvah, I am a colleague of @yannick-softwerft and I finally found some time to continue work on this PR.

I started writing tests and then I realized that this feature also has to support response fields where the entire path is specified. It is now implemented and ready for review.

@yannick-softwerft yannick-softwerft marked this pull request as ready for review September 27, 2024 08:08
Copy link
Contributor Author

@yannick-softwerft yannick-softwerft left a comment

Choose a reason for hiding this comment

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

LGTM

@shalvah
Copy link
Contributor

shalvah commented Oct 2, 2024

Can you add support in the #[ResponseField] attribute as well? Should be easier, since there's no parsing needed.

@sotdan
Copy link
Contributor

sotdan commented Oct 4, 2024

That was actually working already.

BTW: I think it's kind of a breaking change. required has the default value true in Knuckles\Scribe\Attributes\ResponseField, so if you don't specify required as false in your attribute, the ResponseField is treated as required and is suddenly marked as required in the openapi spec.

I demonstrate this in the changes I just made in tests/Strategies/ResponseFields/GetFromResponseFieldAttributesTest.php.

Maybe the default value of required in the ResponseField attribute has to be changed to false?

@shalvah
Copy link
Contributor

shalvah commented Oct 18, 2024

Hmm, I think it is technically breaking, but likely low impact, so I wouldn't want to hold it back.

@shalvah
Copy link
Contributor

shalvah commented Oct 18, 2024

Thanks!

@shalvah shalvah merged commit a6aee1d into knuckleswtf:master Oct 18, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants