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

Refactor CRUDOperationOptions to a hierarchy of classes #258

Merged
merged 5 commits into from
Sep 16, 2022

Conversation

akudiyar
Copy link
Collaborator

@akudiyar akudiyar commented Sep 13, 2022

  • Make CRUD operation options more agile for further adding support of new tarantool/crud features.
  • Add fluent builders for options classes

I didn't forget about

  • Tests
  • Changelog
  • Documentation

Affects #259

Add base options class and fluent builders for CRUD operation options.
@akudiyar akudiyar force-pushed the refactor-crud-operation-options branch from 285f8a3 to 8b2845e Compare September 13, 2022 20:58
@akudiyar akudiyar requested review from ArtDu and Elishtar September 13, 2022 20:58
ArtDu
ArtDu previously requested changes Sep 14, 2022
Copy link
Contributor

@ArtDu ArtDu left a comment

Choose a reason for hiding this comment

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

I've been trying to merge two Builders in class and came to the conclusion your solution is probably the best way. But we need to add javadocs to Builders because it's too complicated and may misunderstanding or I've never seen this pattern before

Then after merging your PR I will add public methods for ProxyTarantoolSpace operations with CRUD*OperationOptions. 'Cause I think it's the only way we can follow.

Also don't forget to add few words to the Changelog, I know that is internal, but I'm thinking all changes are important

@akudiyar akudiyar dismissed ArtDu’s stale review September 14, 2022 22:23

Changes applied

@akudiyar
Copy link
Collaborator Author

I decided to not add the changelog now because I'm going to push another PR immediately with the batch options and there I can add the changelog entry for all changes.

@akudiyar akudiyar requested a review from ArtDu September 14, 2022 22:25
@akudiyar
Copy link
Collaborator Author

akudiyar commented Sep 14, 2022

NB: we cannot use these CRUD*Options classes for representing the user-defined operation options. These are internal containers for passing the options to tarantool/crud API and they include the values from Conditions, for example. These classes are internal and shouldn't leak to API.

For the API, we will have to prepare another hierarchy of options classes, with different available fields and obviously without CRUD prefix.

@ArtDu
Copy link
Contributor

ArtDu commented Sep 15, 2022

NB: we cannot use these CRUD*Options classes for representing the user-defined operation options. These are internal containers for passing the options to tarantool/crud API and they include the values from Conditions, for example. These classes are internal and shouldn't leak to API.

For the API, we will have to prepare another hierarchy of options classes, with different available fields and obviously without CRUD prefix.

Yep, and passing in Conditions startTuple, offset and limit was a wrong idea. Because we have offset and startTuple options that are unique for each of the functions. Also we have problem with the Condition that we use it not only in select, but for update, upsert and etc. which ones don't use these options and that's weird.

I believe we should have implemented API that as close as possible to the original API and futher will add easy to use constructions.

But now we have what we have. And I agree with you that's we need another Options hierarchy so as not to multiply collisions but we will have exactly same structure like CrudBase*Options except options from Conditions and it's hard to accept

@ArtDu ArtDu merged commit 2f6cef7 into master Sep 16, 2022
@ArtDu ArtDu deleted the refactor-crud-operation-options branch January 12, 2023 11:06
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.

2 participants