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: java client APIC-160 #29

Merged
merged 24 commits into from
Dec 14, 2021
Merged

feat: java client APIC-160 #29

merged 24 commits into from
Dec 14, 2021

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Dec 3, 2021

🧭 What and Why

🎟 JIRA Ticket: APIC-160

I needed the Java client to try the CTS generation, so here it is.
The code is way too complex for what we need, I'll try and simplify it in future PR.
The templates are already modified, I will add the init and retry in future PR to make it clearer.

Changes included:

  • Temporary edit the spec to remove oneOf using scripts
  • Load java template
  • Create java client

🧪 Test

Make sur you have java>=8

You need to install maven

brew install maven

Then create the jar

nvm use && yarn install && yarn generate:java:search
yarn client:build-java

And run the playground:

yarn playground:java

@millotp millotp self-assigned this Dec 3, 2021
@shortcuts shortcuts requested review from a team and damcou and removed request for a team December 8, 2021 14:03
@shortcuts
Copy link
Member

(just tested the auto tagging review :D)

@millotp millotp marked this pull request as ready for review December 8, 2021 14:04
@shortcuts
Copy link
Member

So much better with the formatting! Should we review this PR anytime soon or will you iterate with stacked PRs?

@millotp
Copy link
Collaborator Author

millotp commented Dec 9, 2021

I will do stack PR but I prefer to merge PR one by one, but this is still not working because of oneOf, and it doesn't look like OpenAPI is trying to improve it (although it's been two years since this issue), but it's in their short-term roadmap

Copy link
Contributor

@sbellone sbellone left a comment

Choose a reason for hiding this comment

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

Modify the spec a bit to accommodate to java, I need to fix this

Can you point me on the file? I wanted to have a look but I'm not familiar with the repo and there are too many files in the review :)

@millotp millotp requested a review from shortcuts December 14, 2021 10:30
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.

I don't know java so I did not checked the code in detail, but this looks cool overall!

TLDR:

  • Few comments are broken due to special chars
  • I think we can remove the licenseinfo on the classes, it will be stated globally already

@@ -0,0 +1,22 @@
#!/bin/bash
Copy link
Member

Choose a reason for hiding this comment

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

Those scripts should've been in a stacked PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Otherwise this wouldn't compile

Copy link
Member

Choose a reason for hiding this comment

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

Yep but you can base a PR on this one and merge it afterwards, it make the review easier


# Remove the oneOf in the spec until it's supported (https://github.com/OpenAPITools/openapi-generator/issues/10880).

# Remove the oneOf and only keep the last $ref
Copy link
Member

Choose a reason for hiding this comment

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

How do we ensure that the last ref is always the one we want to generate?

Should we add a keyword in our naming until this is fixed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think this is very hackish and unique case, and must be repeated for every new situation, so we don't have to ensure any special syntax. Do you think we should generalize this ?

Copy link
Member

Choose a reason for hiding this comment

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

I can imagine a case where the last oneOf is the searchParamsAsString for example and the only typed/generated code is the one that is less used etc.

We should either make sure our last ref is always the best one or add a keyword that we can use in the regex

@@ -56,9 +56,29 @@
"npmName": "@algolia/client-personalization",
"packageName": "@algolia/client-personalization",
"npmVersion": "5.0.0",

Copy link
Member

Choose a reason for hiding this comment

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

The newline allow us to differentiate the custom variables than the generator ones, we will (sadly) have more and more with the upcoming PRs

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

vscode is autoformating and removing the newline, can we find another way to separate ? Like: customProps: true?

Copy link
Member

Choose a reason for hiding this comment

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

Weird, it does not do it on mine. It's not a big deal I guess, just makes it easier to read

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's because I have format on save checked.

}

/**
* The `facet` name.
Copy link
Member

Choose a reason for hiding this comment

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

Some special chars are broken

Copy link
Collaborator Author

@millotp millotp Dec 14, 2021

Choose a reason for hiding this comment

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

We still have some escaped quote in comments (not necessary), but it's in the js client too, this will be hard to fix

Copy link
Member

Choose a reason for hiding this comment

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

Yep definitely

@@ -0,0 +1,70 @@
# algoliasearch-client-java-2
Copy link
Member

Choose a reason for hiding this comment

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

That's cool!

@millotp millotp requested a review from shortcuts December 14, 2021 14:43
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.

Good to go imo!

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.

3 participants