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(php): Add tests for query params #475

Merged
merged 10 commits into from
May 11, 2022
Merged

Conversation

damcou
Copy link
Contributor

@damcou damcou commented May 5, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-456

Changes included:

🧪 Test

yarn docker cts run php

@netlify
Copy link

netlify bot commented May 5, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 350a33e
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/627b838ab187600009aece90

@algolia-bot
Copy link
Collaborator

algolia-bot commented May 5, 2022

✗ The generated branch has been deleted.

If the PR has been merged, you can check the generated code on the main branch.

@damcou damcou changed the title php(feat): Add tests for query params feat(php): Add tests for query params May 5, 2022
@damcou damcou self-assigned this May 5, 2022
@shortcuts shortcuts mentioned this pull request May 6, 2022
Copy link
Member

@shortcuts shortcuts left a comment

Choose a reason for hiding this comment

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

Nice :D

specs/search/common/parameters.yml Show resolved Hide resolved
@shortcuts
Copy link
Member

CI failing will actually be fixed in #482

@shortcuts shortcuts force-pushed the feat/APIC-456/request-options-php branch from 22c18b1 to 1ca1d34 Compare May 10, 2022 14:46
@damcou damcou requested a review from millotp May 10, 2022 14:57
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Nice !
It's surprising that it doesn't change anything in the java CTS.
I guess the next step is to add header to the CTS !

Comment on lines 178 to 180
if ({{paramName}} !== undefined) {
headers['{{baseName}}'] = {{paramName}};
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is equivalent to before, is this needed ?
You should also add toString() at the end just in case

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes because now the param can be added both in the headers and the query params

tests/output/php/src/methods/requests/RecommendTest.php Outdated Show resolved Hide resolved
@damcou damcou requested a review from millotp May 11, 2022 08:20
@millotp
Copy link
Collaborator

millotp commented May 11, 2022

Looks like something is wrong with the CI checking diff, you forgot to generate the Java CTS.
You can merge main and run CI=true gi checkout main -- tests/output && git add . && git merge -m "restore" and let the CI generate the CTS for you.

@damcou damcou added dependencies Pull requests that update a dependency file and removed dependencies Pull requests that update a dependency file labels May 11, 2022
Copy link
Collaborator

@millotp millotp left a comment

Choose a reason for hiding this comment

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

Perfect :)

@damcou damcou merged commit 954efa8 into main May 11, 2022
@damcou damcou deleted the feat/APIC-456/request-options-php branch May 11, 2022 09:46
This was referenced May 19, 2022
@shortcuts shortcuts mentioned this pull request May 23, 2022
3 tasks
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.

4 participants