Skip to content

Conversation

mpintaric55334
Copy link
Contributor

@mpintaric55334 mpintaric55334 commented Aug 3, 2023

[master < Task] PR

  • Check, and update documentation if necessary
  • Provide the full content or a guide for the final git message
    For util.md5 function in c++ util query module, a need for ToString method arised, so it was implemented.

To keep docs changelog up to date, one more thing to do:

  • Write a release note here, including added/changed clauses
    Users can now call ToString() method on mgp::Value and Memgraph's data types when writing C++ query modules.
  • Tag someone from docs team in the comments
@mpintaric55334 mpintaric55334 added the Ready for review PR is ready for review label Aug 3, 2023
@mpintaric55334 mpintaric55334 self-assigned this Aug 3, 2023
@mpintaric55334 mpintaric55334 changed the title [master < ] Add ToString for mgp::Value and all mgp types [master < T1140] Add ToString for mgp::Value and all mgp types Aug 3, 2023
@mpintaric55334
Copy link
Contributor Author

@vpavicic will write documentation when PR is reviewed.

Copy link
Contributor

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

Few comments, but good work

@antoniofilipovic antoniofilipovic added Needs change PR needs change and removed Ready for review PR is ready for review labels Aug 7, 2023
@mpintaric55334 mpintaric55334 added Ready for review PR is ready for review and removed Needs change PR needs change labels Aug 8, 2023
@mpintaric55334 mpintaric55334 added the Ready for review PR is ready for review label Aug 14, 2023
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Left a few comments. The suggestion to use string::append because the ToString methods build a std::string applies to all methods here.

Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

Looks pretty good! Left a few comments related to formatting and testing.

@antepusic antepusic added Needs change PR needs change and removed Ready for review PR is ready for review labels Aug 16, 2023
@mpintaric55334 mpintaric55334 force-pushed the add-ToString-for-Value-cpp-api branch from 622c9e4 to b4cfbe7 Compare August 16, 2023 11:58
@mpintaric55334 mpintaric55334 added Ready for review PR is ready for review and removed Needs change PR needs change labels Aug 16, 2023
Copy link
Contributor

@antepusic antepusic left a comment

Choose a reason for hiding this comment

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

I think we're good to go here 🚀
Left a small suggestion, but the PR is approvable either way.

@mpintaric55334 mpintaric55334 force-pushed the add-ToString-for-Value-cpp-api branch from 0aee0ca to c9096c3 Compare August 18, 2023 08:02
Copy link
Contributor

@antoniofilipovic antoniofilipovic left a comment

Choose a reason for hiding this comment

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

LGTG

@antoniofilipovic antoniofilipovic added Ship it PR ready to be merged and removed Ready for review PR is ready for review labels Aug 29, 2023
@antoniofilipovic antoniofilipovic merged commit d516e40 into master Aug 29, 2023
@antoniofilipovic antoniofilipovic deleted the add-ToString-for-Value-cpp-api branch August 29, 2023 15:30
@vpavicic vpavicic added the Docs needed Docs needed label Sep 12, 2023
@mpintaric55334 mpintaric55334 added this to the mg-v2.11.0 milestone Sep 12, 2023
as51340 pushed a commit that referenced this pull request Nov 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Docs needed Docs needed Ship it PR ready to be merged

5 participants