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

fix(spec): better filters type #413

Merged
merged 18 commits into from
Apr 26, 2022
Merged

fix(spec): better filters type #413

merged 18 commits into from
Apr 26, 2022

Conversation

shortcuts
Copy link
Member

@shortcuts shortcuts commented Apr 21, 2022

🧭 What and Why

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

closes #177

Changes included:

  • Fix filters related options types
  • Fix searchParams missing options and types

(idk what happened with #227, but it was closed after rebase/force pushing)

🧪 Test

CI :D

@netlify
Copy link

netlify bot commented Apr 21, 2022

Deploy Preview for api-clients-automation ready!

Name Link
🔨 Latest commit 6a8db07
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/6267ebaaa48af40008f60741
😎 Deploy Preview https://deploy-preview-413--api-clients-automation.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 21, 2022

✗ The generated branch has been deleted.

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

@shortcuts shortcuts changed the base branch from main to fix/js-template April 22, 2022 15:02
Comment on lines +493 to +496
// Somehow this is not yet enough
if (oneOf != null && !oneOf.isEmpty()) {
return oneOf.get(0);
}
Copy link
Member Author

Choose a reason for hiding this comment

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

This takes the first item of the list, it's not correct but allow the CI to work for now

@shortcuts shortcuts marked this pull request as ready for review April 25, 2022 09:49
@shortcuts shortcuts requested review from a team, damcou and millotp and removed request for a team April 25, 2022 09:49
@shortcuts shortcuts self-assigned this Apr 25, 2022
Base automatically changed from fix/js-template to main April 25, 2022 09:58
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 ! closer to the true API !

@@ -466,7 +494,18 @@ private IJsonSchemaValidationProperties findMatchingOneOf(
return bestOneOf;
}
if (param instanceof List) {
// no idea for list
// NICE ---> no idea for list <--- NICE
Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry for this, I'll right better error exception next time

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah actually I like this way of thinking!!

@shortcuts shortcuts requested a review from millotp April 25, 2022 16:01
@@ -222,7 +222,7 @@ jobs:
with:
path: |
${{ format('{0}/algoliasearch-core/src/com/algolia/api/{1}.java', matrix.client.path, matrix.client.api) }}
${{ format('{0}/algoliasearch-core/src/com/algolia/model/{1}/**', matrix.client.path, matrix.client.capitalizedName) }}
${{ format('{0}/algoliasearch-core/src/com/algolia/model/{1}/**', matrix.client.path, matrix.client.name) }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

it's not name, it's camelCase, not sure it's available

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhh yes good catch I did not thought of the querySuggestions ones for example

Copy link
Member Author

Choose a reason for hiding this comment

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

millotp
millotp previously approved these changes Apr 26, 2022
*
* `search-client`, `javascript` -> `searchClient`.
*/
export function createClientName(client: string, language: string): string {
Copy link
Collaborator

Choose a reason for hiding this comment

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

much cleaner like that 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

😊

Copy link
Member Author

Choose a reason for hiding this comment

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

(hopefully it works now D:)

Copy link
Collaborator

Choose a reason for hiding this comment

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

except the cache got skipped ahah

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah yes it ran just before ._. oh god

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah no it's good!!!

Copy link
Contributor

Choose a reason for hiding this comment

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

Do I need to change something on my PHP test branch ? :o

Copy link
Member Author

Choose a reason for hiding this comment

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

Not yet but you should merge first, there's still an issue with the CI for Java, I'll come back to it later

@shortcuts shortcuts marked this pull request as draft April 26, 2022 10:43
@shortcuts shortcuts marked this pull request as ready for review April 26, 2022 12:06
millotp
millotp previously approved these changes Apr 26, 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.

Youpi let's go ! 💯

@shortcuts
Copy link
Member Author

Hmm the deleted files are still not pushed...................

@millotp
Copy link
Collaborator

millotp commented Apr 26, 2022

You can do it in another PR, this one is already packed ahah

@shortcuts
Copy link
Member Author

You can do it in another PR, this one is already packed ahah

Yup I won't do it in this one, just testing locally why

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.

[search]: updating reRankingApplyFilter type and adding new property to RankingInfo
4 participants