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

[wokdlhX3] Extract code from APOC core which is only used in APOC extended #261

Merged
merged 12 commits into from
Jan 16, 2023

Conversation

Lojjs
Copy link
Contributor

@Lojjs Lojjs commented Dec 5, 2022

Should go in together with neo4j-contrib/neo4j-apoc-procedures#3361

@Lojjs Lojjs force-pushed the dev-move-parts-of-common-to-extended branch 29 times, most recently from 297611d to f4b9217 Compare December 9, 2022 15:37
@Lojjs Lojjs marked this pull request as ready for review December 9, 2022 15:37
Copy link
Contributor

@conker84 conker84 left a comment

Choose a reason for hiding this comment

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

Thank you so much @Lojjs, just a couple of things that I want to discuss with you

@@ -11,6 +11,7 @@

import static java.net.HttpURLConnection.HTTP_OK;

@SuppressWarnings("unused") // used from extended
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer moving this on extended as it's used only in there as you did for many other APIs

@@ -5,6 +5,7 @@

import java.time.Duration;

@SuppressWarnings("unused") // used from extended
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as GoogleCloudStorageContainerExtension

@@ -139,23 +139,4 @@ private void requireConnected(Relationship relationship) {
throw new IllegalArgumentException("Relationship is not part of current path.");
}
}

public static final class Builder {
Copy link
Contributor

Choose a reason for hiding this comment

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

I prefer to have it in there, now it's used only in DataVirtualizationCatalog but by design would be better having it in there; as in the future could be used in other use-cases

@conker84
Copy link
Contributor

@Lojjs can you please rebase it?

@Lojjs Lojjs force-pushed the dev-move-parts-of-common-to-extended branch from 4e80267 to 258772a Compare January 11, 2023 13:43
@Lojjs Lojjs force-pushed the dev-move-parts-of-common-to-extended branch from 46c818c to ea01825 Compare January 11, 2023 15:30
@conker84
Copy link
Contributor

@Lojjs LGTM thank you so much!

@Lojjs Lojjs merged commit 3950300 into dev Jan 16, 2023
@Lojjs Lojjs deleted the dev-move-parts-of-common-to-extended branch January 16, 2023 11:36
@Lojjs Lojjs restored the dev-move-parts-of-common-to-extended branch January 16, 2023 11:36
@Lojjs Lojjs deleted the dev-move-parts-of-common-to-extended branch January 16, 2023 11:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants