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(java): move code to src folder APIC-411 #387

Merged
merged 7 commits into from
Apr 21, 2022
Merged

Conversation

millotp
Copy link
Collaborator

@millotp millotp commented Apr 19, 2022

🧭 What and Why

🎟 JIRA Ticket: APIC-411

In preparation for the java release, move the code to src folder to have it's own build.gradle file.

Changes included:

  • Move src

🧪 Test

CI

@millotp millotp self-assigned this Apr 19, 2022
@netlify
Copy link

netlify bot commented Apr 19, 2022

Deploy Preview for api-clients-automation canceled.

Name Link
🔨 Latest commit 69c964f
🔍 Latest deploy log https://app.netlify.com/sites/api-clients-automation/deploys/626114bc634fdf0008acb71f

@algolia-bot
Copy link
Collaborator

algolia-bot commented Apr 19, 2022

✗ The generated branch has been deleted.

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

@shortcuts
Copy link
Member

Could we do this PR on top of #385 so it uses the next logic in place? 😇

@millotp
Copy link
Collaborator Author

millotp commented Apr 19, 2022

#385 will need rebasing and it will be a mess, I will make the changes once it's merged

@millotp millotp force-pushed the fix/move-src-java branch 3 times, most recently from cbb9a24 to c66c04e Compare April 20, 2022 13:43
@millotp millotp requested review from a team and shortcuts and removed request for a team April 20, 2022 13: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.

This looks good! I just have one question: Is the other src folder usable or do we need the algoliasearch-core one?

@millotp
Copy link
Collaborator Author

millotp commented Apr 20, 2022

Actually everything should be inside algoliasearch-core/src/ now, should I delete the files by hand ?

@shortcuts
Copy link
Member

Actually everything should be inside algoliasearch-core/src/ now, should I delete the files by hand ?

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))

@shortcuts
Copy link
Member

I was mostly wondering because we have algoliasearch-client-java-2/src already and I don't know what it's used for

@millotp
Copy link
Collaborator Author

millotp commented Apr 20, 2022

It's not used for anything, it's empty and not commited, maybe I can remove it with the custom gen

@shortcuts
Copy link
Member

160a4ce will delete and push correctly

@shortcuts
Copy link
Member

shortcuts commented Apr 20, 2022

Hmmm I believe there's no way for the CI to know that the whole folder has moved from / to /src... you'll maybe need to mv it manually :(

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.

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

@millotp
Copy link
Collaborator Author

millotp commented Apr 21, 2022

No it's not correct still, the CI passed with cache but there a some files missing

@millotp millotp marked this pull request as draft April 21, 2022 07:57
@millotp millotp marked this pull request as ready for review April 21, 2022 08:39
@millotp millotp requested a review from shortcuts April 21, 2022 08:40
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.

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();
Copy link
Member

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?

Copy link
Member

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)

Copy link
Member

@shortcuts shortcuts Apr 21, 2022

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

Copy link
Collaborator Author

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.

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