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(generator): handle custom request escaping #313

Merged
merged 7 commits into from
Apr 4, 2022
Merged

fix(generator): handle custom request escaping #313

merged 7 commits into from
Apr 4, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Mar 30, 2022 •
edited
Loading

🧭 What and Why

🎟 JIRA Ticket: https://algolia.atlassian.net/browse/APIC-350

Changes included:

Follow up of #315

  • Add the new x-is-custom-request variable to the templates to avoid encoding path parameters of custom requests
  • Remove esm from bundlesize
    • Sorry, it's not related
  • Fix cache path for the PHP client

🧪 Test

CI :D

@shortcuts shortcuts self-assigned this Mar 30, 2022
@netlify
Copy link

netlify bot commented Mar 30, 2022 •
edited
Loading

✅ Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 370a7ac
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/624aed438f98ff0008a65c35

@algolia-bot
Copy link
Collaborator

algolia-bot commented Mar 30, 2022 •
edited
Loading

✗ The generated branch has been deleted.

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

@shortcuts shortcuts mentioned this pull request Mar 30, 2022
Merged
@millotp millotp mentioned this pull request Mar 31, 2022
Merged
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from ffcf747 to 910b62e Compare March 31, 2022 09:19
Base automatically changed from fix/javascript-comments-jsdoc to main March 31, 2022 09:58
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from 910b62e to 6008884 Compare March 31, 2022 10:29
@shortcuts shortcuts changed the title feat(cts): add custom request tests feat(generator): handle custom request escaping Mar 31, 2022
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from 6008884 to b81a580 Compare March 31, 2022 12:07
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from b81a580 to a1396b9 Compare March 31, 2022 12:08
@shortcuts shortcuts mentioned this pull request Mar 31, 2022
Merged
@shortcuts
Copy link
Member Author

#318 should fix the generation for the PHP client

@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from 87e0614 to 5cebd4a Compare April 4, 2022 11:10
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from 5cebd4a to 5a79f54 Compare April 4, 2022 11:30
@shortcuts shortcuts requested review from a team, eunjae-lee and damcou and removed request for a team April 4, 2022 11:40
@shortcuts shortcuts marked this pull request as ready for review April 4, 2022 11:40
@eunjae-lee
Copy link
Contributor

I think the title should be fix(generator) because it's not adding any feature but fixing the escape issue.

@shortcuts shortcuts changed the title feat(generator): handle custom request escaping fix(generator): handle custom request escaping Apr 4, 2022
millotp
millotp reviewed Apr 4, 2022
View reviewed changes
.github/actions/cache/action.yml
@@ -382,7 +384,7 @@ runs:
if: ${{ inputs.job == 'cts' || inputs.job == 'codegen' }}
uses: actions/cache@v2
with:
path: clients/algoliasearch-client-php
path: clients/algoliasearch-client-php/lib/Api/RecommendApi.php
Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think this will work once we remove the generated code, this is not the only generated file, there are other as listed here,
the easiest is to keep clients/algoliasearch-client-php

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think this will work once we remove the generated code

This should be tackled once we encounter the issue

the easiest is to keep clients/algoliasearch-client-php

The cache contains the whole folder, not only the changes, so the latest restoration will override all the previous ones

Copy link
Collaborator

@millotp millotp Apr 4, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think we should break something to fix it later, is it an issue right now ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

The cache is restored only if nothing has changed in theory, I don't see the issue

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think we should break something to fix it later, is it an issue right now ?

It does not seem to work right now

This is the commit for the same generated code for same cache path: 1c27d48

@shortcuts shortcuts mentioned this pull request Apr 4, 2022
Merged
@shortcuts shortcuts requested a review from millotp April 4, 2022 12:54
@shortcuts shortcuts force-pushed the feat/customRequest-cts branch from 32e25c8 to 558cbf0 Compare April 4, 2022 13:06
millotp
millotp approved these changes Apr 4, 2022
View reviewed changes
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.

Let's go !

@shortcuts shortcuts merged commit eeeeea8 into main Apr 4, 2022
@shortcuts shortcuts deleted the feat/customRequest-cts branch April 4, 2022 13:25
algolia-bot added a commit to algolia/algoliasearch-client-javascript that referenced this pull request Apr 4, 2022
@algolia-bot @shortcuts
fix(generator): handle custom request escaping
772896f 
algolia/api-clients-automation#313

Co-authored-by: Clément Vannicatte <[email protected]>
algolia-bot added a commit to algolia/algoliasearch-client-java that referenced this pull request Apr 4, 2022
@algolia-bot @shortcuts
fix(generator): handle custom request escaping
1c200a8 
algolia/api-clients-automation#313

Co-authored-by: Clément Vannicatte <[email protected]>
algolia-bot added a commit to algolia/algoliasearch-client-php that referenced this pull request Apr 4, 2022
@algolia-bot @shortcuts
fix(generator): handle custom request escaping
637e1c3 
algolia/api-clients-automation#313

Co-authored-by: Clément Vannicatte <[email protected]>
@shortcuts shortcuts mentioned this pull request Apr 4, 2022
Merged
shortcuts added a commit that referenced this pull request Apr 22, 2022
@shortcuts
fix(generator): handle custom request escaping (#313)
70a7ffa
This was referenced May 24, 2022
Closed
Closed
Closed
Closed
Closed
Closed
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@millotp millotp millotp approved these changes

@eunjae-lee eunjae-lee Awaiting requested review from eunjae-lee eunjae-lee is a code owner automatically assigned from algolia/api-clients-automation

@damcou damcou Awaiting requested review from damcou damcou is a code owner automatically assigned from algolia/api-clients-automation

Assignees

@shortcuts shortcuts

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shortcuts @algolia-bot @eunjae-lee @millotp