Skip to content

Conversation

@millotp
Copy link
Contributor

@millotp millotp commented Aug 22, 2024

🧭 What and Why

Generate snippets for helpers, we might want to add isSnippet too to only select the best looking one, but for now its fine.
I had to refacto the SnippetsGenerator and stop sharing code with the TestsRequest class because it was too messy with too many branches.

@millotp millotp self-assigned this Aug 22, 2024
@millotp millotp requested a review from a team as a code owner August 22, 2024 14:32
@millotp millotp requested review from Fluf22 and morganleroi August 22, 2024 14:32
@algolia-bot
Copy link
Collaborator

algolia-bot commented Aug 22, 2024

✔️ Code generated!

Name Link
🪓 Triggered by 7c8a8dd77f57036312c586e68938367e3dfef09e
🍃 Generated commit 647ac340b8cc1457d5f3b953fe579e1e32827ede
🌲 Generated branch generated/chore/gen-snippets
📊 Benchmark results

Benchmarks performed on the method using a mock server, the results might not reflect the real-world performance.

Language Req/s
javascript 1620
php 1468
csharp 1233
python 978
ruby 931
java 923
swift 770
go 570
kotlin 483

@github-actions
Copy link

github-actions bot commented Aug 22, 2024

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.

this looks wonderful, small question about algoliasearch

@millotp millotp force-pushed the chore/gen-snippets branch from bd785da to 13161b9 Compare August 29, 2024 14:28
@millotp millotp requested a review from shortcuts August 30, 2024 00:01
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.

Looks awesome! I only have nitpick comments so non blocking, will check the gen

Map<String, Snippet[]> snippets = loadFullCTS(Snippet[].class);

String clientName = client;
if (client.equals("algoliasearch")) {
Copy link
Member

Choose a reason for hiding this comment

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

We should rename this. lite as we discussed, but in an other pr

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep agree

protected <T> Map<String, T[]> loadFullCTS(Class<T[]> jsonType) throws Exception {
String clientName = client;
// This special case allow us to read the `search` CTS to generated the tests for the
// `lite` client, which is only available in Javascript
Copy link
Member

Choose a reason for hiding this comment

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

It's in dart too no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

true, this is an old comment


Map<String, T[]> baseCTS = loadCTS("requests", clientName, jsonType);

// The algoliasearch client bundles many client and therefore should provide tests for all the
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The algoliasearch client bundles many client and therefore should provide tests for all the
// The lite client bundles many client and therefore should provide tests for all the

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do all the renaming in another PR

@shortcuts
Copy link
Member

Can we make the generator replace the values of indexName params with something like YOUR_INDEX_NAME?

@shortcuts
Copy link
Member

Gen looks incredible

@millotp
Copy link
Contributor Author

millotp commented Aug 30, 2024

Can we make the generator replace the values of indexName params with something like YOUR_INDEX_NAME?

I tried a little hack to make this work, assuming the parameter is always called indexName, it's already way better !

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.

💅🏻 CLEANNNN

@millotp millotp merged commit 742c6d7 into main Sep 2, 2024
@millotp millotp deleted the chore/gen-snippets branch September 2, 2024 07:57
algolia-bot added a commit that referenced this pull request Sep 2, 2024
…kip ci]

Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
algolia-bot added a commit to algolia/algoliasearch-client-scala that referenced this pull request Sep 2, 2024
algolia/api-clients-automation#3575

Co-authored-by: algolia-bot <accounts+algolia-api-client-bot@algolia.com>
Co-authored-by: Pierre Millot <pierre.millot@algolia.com>
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