marzer / tomlplusplus

Header-only TOML config file parser and serializer for C++17.
https://marzer.github.io/tomlplusplus/
MIT License
1.53k stars 146 forks source link

Make key::str() to return std::string instead of std::string_view? #203

Closed yihuaeyang closed 1 year ago

yihuaeyang commented 1 year ago

Is your feature request related to a problem? Please describe.

I'd like to concatenate the underlying std::string of a toml::key with other c_str constants ("..."), or use it as a key to other mappings. Existing key::str() returns std::string_view which requires constructing a new std::string for simple concatenation with +, or to be used as a key to a std::map<std::string, T>, for example.

Describe the solution you'd like

Make key::str() return a constant reference of the underlying key_. The properly named key::string_view() can still return a std::string_view.

Additional context

It would also be more consistent for the str() method in to return std::string, for example, as in std::stringstream::str.

marzer commented 1 year ago

It returns a string view because when I added toml::key I thought that it may not always be backed by a std::string, and instead by some other central 'key buffer' or somesuch, with contiguous storage. This has not come to be, but who knows, it might still?

The properly named key::string_view()

The current name is 'proper', thank you very much. It returns a string in the conceptual sense. The underlying storage is irrelevant.

or to be used as a key to a std::map<std::string, T>, for example.

You can use toml::key directly as the key type in a map, FYI.

It would also be more consistent for the str() method in to return std::string

That method was added in a much earlier version of C++, long before std::string_view was a part of the standard library. I think it's pretty widely accepted nowadays that streams in general are not a good example of API design, so forgive me if I don't find that particularly compelling.

In any case, changing this would be an ABI break for everyone currently using the library, which does not seem worth it for a relatively minor QoL improvement for your specific need.

yhey commented 1 year ago

The current name is 'proper', thank you very much. It returns a string in the conceptual sense. The underlying storage is irrelevant.

Proper or not, it is strange to have a method named str to return a string_view.

or to be used as a key to a std::map<std::string, T>, for example.

You can use toml::key directly as the key type in a map, FYI.

No, I cannot, because my map consists of all kinds of string keys, which may or may not have been set in the parsed TOML. I do not want to create a toml::key for every possible string that could exist in my map.

It would also be more consistent for the str() method in to return std::string

That method was added in a much earlier version of C++, long before std::string_view was a part of the standard library. I think it's pretty widely accepted nowadays that streams in general are not a good example of API design, so forgive me if I don't find that particularly compelling.

It is a logical fallacy to assume everything about the streams library is bad simply because part of it doesn't show good examples of API design.

In any rate, it should be pretty obvious to anyone that the naming of str() to return std::string in std::stringstream is not a problem.

In any case, changing this would be an ABI break for everyone currently using the library, which does not seem worth it for a relatively minor QoL improvement for your specific need.

This might be the only excuse with some weight against a change. However, I doubt changing the return type of std::string_view into std::string& would cause any problem, since AFAIK, anything std::string_view can do, std::string& can, too. But if you have a good counter example, I'm all ears.

Buy hey, it's your project. If you like the way it is, then so be it. Please don't get offended. I like your project and appreciate your work. Thanks.

marzer commented 1 year ago

This might be the only excuse

My response wasn't excuses, it was my reasoning behind the design decision I made. I wholesale disagree with pretty much everything you said, and my reasoning in my original response is sufficient.

Please don't get offended

The whole tone of this issue and your response to it is weirdly demanding and condescending, so yeah I'm a bit offended by it tbh, and it's perfectly reasonable that I would be. Do you go around being condescending to, and arguing with, all OSS maintainers you want something from? We don't owe you anything.

marzer commented 1 year ago

Also,

However, I doubt changing the return type of std::string_view into std::string& would cause any problem [...]

I said ABI break, not API break.