Skip to content

Make mapper builder more universal#391

Merged
akudiyar merged 10 commits intomasterfrom
change-builder
Jun 2, 2023
Merged

Make mapper builder more universal#391
akudiyar merged 10 commits intomasterfrom
change-builder

Conversation

@ArtDu
Copy link
Copy Markdown
Contributor

@ArtDu ArtDu commented May 2, 2023

I haven't forgotten about:

  • Tests
  • Changelog
  • Documentation
  • Commit messages comply with the guideline
  • Cleanup the code for review. See checklist

Related issues:

Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

Changing the call method signature seems reasonable, but that's a breaking change and we must notify the customers appropriately. Changing MessagePackMapper parameter types to MessagePackValueMapper also seems reasonable.

But we shouldn't have any raw Objects in the API (List<?> in call and eval is the only exception here because of the compatibility with tarantool-java and also it's a better variant than just Object). ALso I don't get why we change CallResultMapper result types to something else.

@ArtDu ArtDu force-pushed the change-builder branch from 3394e4c to 9449a65 Compare May 25, 2023 17:09
@ArtDu ArtDu changed the title WIP Make mapper builder more universal May 25, 2023
@ArtDu
Copy link
Copy Markdown
Contributor Author

ArtDu commented May 25, 2023

But we shouldn't have any raw Objects in the API (List<?> in call and eval is the only exception here because of the compatibility with tarantool-java and also it's a better variant than just Object). ALso I don't get why we change CallResultMapper result types to something else.

I added two ways of using with generics and without. Check them.
We need object for universal mapping when return type is uncertain.

@ArtDu ArtDu requested a review from akudiyar May 25, 2023 17:16
@ArtDu ArtDu self-assigned this May 25, 2023
@ArtDu ArtDu force-pushed the change-builder branch from cf47d8d to e81b336 Compare May 31, 2023 22:16
@ArtDu ArtDu marked this pull request as ready for review May 31, 2023 22:22
@ArtDu
Copy link
Copy Markdown
Contributor Author

ArtDu commented May 31, 2023

@akudiyar I probably did all what we discussed. And also I updated PR in springdata(tarantool/cartridge-springdata#124). Please check both PR and give your opinion

akudiyar
akudiyar previously approved these changes Jun 1, 2023
Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

That's in the much better shape now. I have just three small nitpicks

ArtDu and others added 2 commits June 2, 2023 14:02
Co-authored-by: Alexey Kuzin <akudiyar@gmail.com>
@ArtDu ArtDu force-pushed the change-builder branch from 30a692e to 7810c1e Compare June 2, 2023 11:02
Copy link
Copy Markdown
Collaborator

@akudiyar akudiyar left a comment

Choose a reason for hiding this comment

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

LGTM

@akudiyar akudiyar added this pull request to the merge queue Jun 2, 2023
Merged via the queue into master with commit f71d187 Jun 2, 2023
@akudiyar akudiyar deleted the change-builder branch June 2, 2023 22:15
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