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

NO AUTO Renames core to common package, makes core and full depend on it #3079

Merged
merged 5 commits into from
Aug 1, 2022

Conversation

ncordon
Copy link
Contributor

@ncordon ncordon commented Jul 29, 2022

What

Renames core to common, creates a new core package and makes core and full depend on common.

Why

This is a step towards splitting core and full completely. Next step should move procedures from common to core, increasingly.

Next

  • Processor dependency in core? At the moment it's left in common
  • core->test depends on common->test. Should we remove that in the future?
  • Fix some procedures / functions not being available in full.

@ncordon ncordon added 5.0 team-cypher-surface Cypher Surface team should review the PR labels Jul 29, 2022
Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

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

Really nice work so far. I had a couple of small suggestions.

I also have a question regarding the core tests. It seems like those tests are currently not run, or at least I couldn't find any evidence that they were. Do you think that is indeed the case?

core/src/test/java/apoc/ApocSplitTest.java Outdated Show resolved Hide resolved
full/src/test/java/apoc/ApocSplitTest.java Outdated Show resolved Hide resolved
minHeapSize = '128m'
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think we could remove this completely in the 3 common/core/processor build.gradle files?

I'm not sure the tests are particularly parallelizable unfortunately, even when run locally.

Copy link
Contributor Author

@ncordon ncordon Aug 1, 2022

Choose a reason for hiding this comment

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

I've removed it, so let's see, but I'm not a fan of introducing unnecessary changes in PRs and this was pre-existing and if it makes the CI fail I will undo.

@ncordon ncordon changed the title Renames core to common package, makes core and full depend on it NO AUTO Renames core to common package, makes core and full depend on it Aug 1, 2022
@ncordon
Copy link
Contributor Author

ncordon commented Aug 1, 2022

I also have a question regarding the core tests. It seems like those tests are currently not run, or at least I couldn't find any evidence that they were

They are running, but they contain only docker tests which are ignored (unfortunately at the moment)

Copy link
Contributor

@AzuObs AzuObs left a comment

Choose a reason for hiding this comment

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

Great job 🎉

@ncordon ncordon merged commit e9d5148 into dev Aug 1, 2022
@ncordon ncordon deleted the dev-split-core-full branch August 1, 2022 15:43
@ncordon ncordon added the split Work related to splitting apoc core and full label Aug 2, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5.0 split Work related to splitting apoc core and full team-cypher-surface Cypher Surface team should review the PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants