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

Missing documentation for dsv() #520

Closed
pauljurczak opened this Issue Oct 19, 2018 · 9 comments

Comments

Projects
None yet
4 participants
@pauljurczak
Copy link

pauljurczak commented Oct 19, 2018

Reference Alphabetical Index is missing dsv and boost::geometry::detail. I'm using them in this code snippet:

    using namespace boost::geometry;
    model::polygon<model::d2::point_xy<unsigned char>> poly{{{0, 0}, {0, 5}, {5, 0}, {0, 0}}};
    std::cout << dsv(poly);
@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Nov 21, 2018

Code in boost::geometry::detail is not documented because these are implementation details. What parts of the library details are you using? I cannot see them in this example.

@barendgehrels Is there a reason why bg::dsv() is not documented? I see that it existed since the acceptance of Boost.Geometry. Was it considered to be not finished (and is still now)? Or is it simply an omission?

@mloskot

This comment has been minimized.

Copy link
Member

mloskot commented Nov 21, 2018

AFAIS, it is documented. It is just missing from the Doxygen input list:

geometry/doc/doxy/Doxyfile

Lines 205 to 208 in 2a2e0ac

../../../../boost/geometry/iterators \
../../../../boost/geometry/io/wkt \
../../../../boost/geometry/io/svg \
../../../../boost/geometry/policies \

I think it's been simply overlooked.

The way documentation is generated was changing couple of times since the review, so possibly one of post-review Doxygen re-reconfiguring lost it (e.g. previously, Doxygen greedily scanned boost/geometry, now it takes explicit list).

@barendgehrels

This comment has been minimized.

Copy link
Collaborator

barendgehrels commented Nov 21, 2018

@mloskot @awulkiew Agree, it's probably been overlooked.
As far as I know it is finished - it is less complex than most of the code we have. The main manipulator is not in the detail namespace so it is meant to be used.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Nov 25, 2018

Actually it seems dsv() was never documented and other IOs were documented in 1.54 (see: https://www.boost.org/doc/libs/1_54_0/libs/geometry/doc/html/index.html vs https://www.boost.org/doc/libs/1_53_0/libs/geometry/doc/html/index.html). I haven't looked why DSV was ommited then.

I'm reluctant to document it now because I don't like the interface. Besides the geometry the function takes other parameters like coordinate separators, list separators etc. so if the user wanted to use different ones then the defaults they would have to be passed to all calls of dsv(). Furthermore not all parts of the DSV string can be defined, e.g. multi-list or polygon-rings separators are missing. What would be perfect is a way of setting up a user-defined state of a stream with custom manipulators, like:

os << bg::dsv_coordinate_separator("|");
// since now this separator will be used in this stream for all geometries
os << bg::dsv(point) << std::endl;
os << bg::dsv(polygon) << std::endl;

but this is AFAIK impossible. Or is it?

The most obvious would be to have our own stream type (maybe wrapper) capable of storing states defined by us but I'd prefer to avoid it since this solution has its own limitations, worse IMO than the original interface.

Anyway, my point is that before documenting it I'd prefer to investigate other ways of writing this interface. With that said I don't know when I'd do this and I doubt it'd be anytime soon. Or we could document it now and in the unknown future announce a breaking change. What do you think?

@barendgehrels

This comment has been minimized.

Copy link
Collaborator

barendgehrels commented Nov 26, 2018

Why change it? It exists for 7 years or more. I don't think you can change it, even though it is not documented on the site (internally it is).
I think we should just leave it as it is, because people might use it already for years. If you want something fancier, and you get it working like that, it has to come with another method name...

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Nov 27, 2018

Ok. So it simply has to be documented then.

For the record, one solution I came up with is using a function object instead of a free function. So:

bg::dsv_definition dsv/*(some_separator1, some_separator2, ...)*/;
dsv.coordinate_separator = "|";
dsv.list_separator = ";";
// etc.

std::cout << dsv(point) << std::endl;
std::cout << dsv(polygon) << std::endl;

However if a user wanted he/she could simply write a new function hiding bg::dsv() call inside, passing required separators. Or in fact the above function object. So this is probably not needed in Boost.Geometry.

@mloskot

This comment has been minimized.

Copy link
Member

mloskot commented Nov 27, 2018

Perhaps this could be displayed as a use case example in the dsv docs.

The dsv is an old facility initially used as a printf-like 'visualisation' for testing and debugging. It predates the SVG. So, it was implemented as very basic.

@barendgehrels

This comment has been minimized.

Copy link
Collaborator

barendgehrels commented Nov 27, 2018

Well, it should also support geoJson, for example. IIRC.

@awulkiew

This comment has been minimized.

Copy link
Member

awulkiew commented Dec 3, 2018

Fixed by #532

@awulkiew awulkiew closed this Dec 3, 2018

@awulkiew awulkiew added this to the 1.69 milestone Dec 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment