- Notifications
You must be signed in to change notification settings - Fork 27
fix(java): move code to src folder APIC-411 #387
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
Conversation
✅ Deploy Preview for api-clients-automation canceled.
|
✗ The generated branch has been deleted.If the PR has been merged, you can check the generated code on the |
| Could we do this PR on top of #385 so it uses the next logic in place? 😇 |
| #385 will need rebasing and it will be a mess, I will make the changes once it's merged |
cbb9a24 to c66c04e Compare There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good! I just have one question: Is the other src folder usable or do we need the algoliasearch-core one?
| Actually everything should be inside |
Last time it did not worked without deleting folder, but let's wait for the CI to see if it does the switch (same issue here: #386 (comment)) |
| I was mostly wondering because we have |
| It's not used for anything, it's empty and not commited, maybe I can remove it with the custom gen |
| 160a4ce will delete and push correctly |
c66c04e to f15520f Compare | Hmmm I believe there's no way for the CI to know that the whole folder has moved from |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As there's no changes in the code itself, you can mv locally and push the changes, we don't have to rely on the CI here
| No it's not correct still, the CI passed with cache but there a some files missing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
N I C E!!
My comments are non blocking
| additionalProperties.put("apiNameSuffix", Utils.API_SUFFIX); | ||
| additionalProperties.put("java8", true); | ||
| additionalProperties.put("sourceFolder", "algoliasearch-core"); | ||
| super.processOpts(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice found!!!
Just found one question: wouldn't it be possible that this is overridden by the gen we extend?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(by that I mean: the settings we've set above)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, there's the JS generator that still call it in the old order
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I wanted to avoid was the duplication of sourceFolder and such, for js there is no duplication so no need to move it.
It's possible that the super.processOpts() overrides some value, but because the generation didn't change I assumed not.
🧭 What and Why
🎟 JIRA Ticket: APIC-411
In preparation for the java release, move the code to
srcfolder to have it's ownbuild.gradlefile.Changes included:
🧪 Test
CI