Skip to content

Conversation

sudo-panda
Copy link
Contributor

@sudo-panda sudo-panda commented Feb 29, 2020

This PR fixes #654 . It adds the following:

  • to_wkt: This function converts the given geometry to wkt string and returns it instead of modifying a stringstream taken as input like wkt.
  • from_wkt: This function returns the geometry obtained by parsing a wkt string instead of taking in a geometry variable as parameter and storing the geometry in it.
  • tests: Tests for both to_wkt and from_wkt are provided with basically follow the template of already implemented read_wkt and wkt tests.
  • examples and documentation: Example usage of both have been added and documentation has been updated.

This PR is now ready to be reviewed and then merged.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

Although needs some changes, it's looking good.

Please, consider also

  • new example in oc/src/examples/io/
  • finding if there is place for documentation (look for .qbk files for other wkt functions)

@sudo-panda sudo-panda requested review from mloskot and awulkiew March 2, 2020 20:15
@sudo-panda sudo-panda changed the title [WIP] Symmetrizing read/write wkt Symmetrizing read/write wkt Mar 2, 2020
Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

This is getting complete. I just pointed out a bunch of issues.

@sudo-panda sudo-panda requested a review from mloskot March 3, 2020 08:48
@vissarion vissarion self-requested a review March 3, 2020 13:55
Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Looks OK to me. Thanks!

@vissarion vissarion self-requested a review March 4, 2020 11:24
@mloskot
Copy link
Member

mloskot commented Mar 5, 2020

@sudo-panda FYI, next time you update your PR, then documentation and doc/src/examples will be built and verified with GitHub Actions.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

@sudo-panda Thanks for another round of your efforts. This to me is looking good, just having two minor requests.

@sudo-panda sudo-panda requested review from mloskot and awulkiew March 8, 2020 09:58
@mloskot
Copy link
Member

mloskot commented Mar 9, 2020

@sudo-panda If you update your PR, you will get #680 change and GitHub Actions will CI check it for you.

Copy link
Member

@mloskot mloskot left a comment

Choose a reason for hiding this comment

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

This looks good to me and ready to merge.
If no objections arrive from @awulkiew or @vissarion in next days, I'll merge it.

I assume @barendgehrels generally agrees on such utility functions as per his #654 (comment) and #654 (comment)

@sudo-panda Thanks for your work!

@sudo-panda
Copy link
Contributor Author

@mloskot thanks for the guidance and approving the PR.

Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

Thanks I am ok with merging.

- Add example demonstrating use of `significant_digits` in `to_wkt`
- Add example demonstrating use of `significant_digits` in `wkt`
- Add parameter documentation
- Add test for `to_wkt` and `wkt`
- Check if significant digit `<0` instead of `==-1`
- Check if type is arithmetic in append function
- Set precision to `digits10` when `T` is specialized and the user doesn't specify the precision
- Set precision to the user supplied precision in all other cases
- Overload function w.r.t. `significant_digits` parameter
- to_wkt() now internally calls wkt()
- minor change: added copyright to read.hpp
Using alias-declaration instead of typedef
@sudo-panda sudo-panda force-pushed the feature/symmetric-wkt branch from cc31ea9 to 0839e31 Compare December 3, 2020 11:39
@vissarion vissarion self-requested a review December 7, 2020 11:19
Copy link
Member

@vissarion vissarion left a comment

Choose a reason for hiding this comment

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

I am also OK with merging most of this PR but (as @barendgehrels ) I do not understand exactly what to_wkt adds on top of the stream operator.

@awulkiew
Copy link
Member

awulkiew commented Dec 7, 2020

@barendgehrels @vissarion This PR implements this issue: #654

@vissarion
Copy link
Member

@barendgehrels @vissarion This PR implements this issue: #654

Right, thanks! So it seems there is user need so I will not go against. I am OK with this PR.

@mloskot
Copy link
Member

mloskot commented Dec 10, 2020

@vissarion, @barendgehrels

Right, thanks! So it seems there is user need so I will not go against. I am OK with this PR.

Yes, there seem to be a need indeed.

Although the lengthy discussion we've had on this PR may given an impression it's a big feature, it is not.
It just a little thing that smoothens up a particular rough edge of the WKT I/O interface making it just a bit more expressive and fluent. Apparently, for users, every little helps :-)

@barendgehrels
Copy link
Collaborator

barendgehrels commented Dec 11, 2020

@vissarion, @barendgehrels

Right, thanks! So it seems there is user need so I will not go against. I am OK with this PR.

Yes, there seem to be a need indeed.

Although the lengthy discussion we've had on this PR may given an impression it's a big feature, it is not.
It just a little thing that smoothens up a particular rough edge of the WKT I/O interface making it just a bit more expressive and fluent. Apparently, for users, every little helps :-)

Thanks for the link (I mean the one with issue #654) . Yes, I remember now and I was OK back then, so I'm still OK now. I will approve.

@awulkiew awulkiew added this to the 1.76 milestone Mar 3, 2021
@awulkiew
Copy link
Member

awulkiew commented Mar 3, 2021

I apologize it took so long. Thanks for this PR!

@awulkiew awulkiew merged commit 28559c0 into boostorg:develop Mar 3, 2021
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.

Proposal: Make read/write WKT more symmetric by adding writing to std::string
5 participants