Skip to content
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

added support and tests for multiline string encoding closes BurntSushi/toml#64 #65

Closed
wants to merge 3 commits into from

Conversation

srinathh
Copy link

I would like to propose a solution to encoding multiline strings and raw multiline strings that I had raised in the issue #64.

The proposal is to accept a new struct tag modifier which can take values of "multiline_string" or "multiline_rawstring" (exported as constants MOD_MULTILINE_STRING and MOD_MULTILINE_RAWSTRING). On detecting presence of either of these modifiers, the type of the struct member is checked to confirm to reflect.String and then the Key.String() to which the modifier applies is inserted into a new map attribute of Encoder called modifiers.

In the functions that actually process and emit the key/value (in this case in keyEqElement) we can check for presence of the key being processed currently in modifiers and if the modifier is MOD_MULTILINE_STRING or MOD_MULTILINE_RAWSTRING, the element is written by the new writeMultiLineString() function. The reason for choosing the route of using an additional encoder attribute is that Key is currently implemented as []string which makes it difficult to store modifiers with the Key itself.

The same concept can be extended to any other output modifiers if necessary even in the future. I have added tests for multiline string encoding currently in the encode_test.go file (not yet figured out the toml-test package) and still need to writeup documentation but would like to seek review of the proposal

@srinathh
Copy link
Author

The build on Travis shows as failed but it seems the failure is on some other problem with an integer related test in the toml-test suite rather than on multiline strings. This is the error message -

Error decoding TOML: Near line 1, key 'answer': Integer '9223372036854776000' is out of the range of 64-bit signed integers.

@BurntSushi
Copy link
Owner

This looks pretty good. I like the idea of using struct tags to do this.

I'll review the commit/strategy in more detail (but at a glance, it looks good).

Also, don't worry about the test failing. I think I know the problem---I'll get it straightened out.

Thank you!

@srinathh
Copy link
Author

A simpler implementation by using just a modifier attribute in Encoder to indicate if the current key being processed has some kind of a modifier seems to work - I've just pushed this.

The previous implementation involved inserting a Key.String() mapped to modifier type into a map[string]string attribute in the Encoder - mainly for safety since I was not confident I was reasoning about the encoder right. However, my understanding now from looking at the code and some playing around is that as soon as a struct element's tags are parsed, it is emitted as toml output immediately. Assuming this understanding is correct, we could just as well use a simple flagging like approach to indicate the modifier.

@srinathh
Copy link
Author

srinathh commented Mar 2, 2015

Hi - anything you'd like me to further check or clean-up on this?

@darethas
Copy link

darethas commented Mar 3, 2016

Has this effort fizzled?

I am interested in this effort.

As an aside, toml 0.4.0 has a syntax for multi line strings with the triple quotes that I think would map nicely to go's raw strings

@cespare
Copy link
Collaborator

cespare commented Mar 3, 2016

@treehau5 I will review this patch when I have some time.

Personally, I'm somewhat opposed to adding a specific feature to support encoding multi-line strings. It seems like an API expansion that doesn't carry its weight. I think that requiring a custom encoder type for this unusual case would be a better option here (see #76).

Note that this is only about encoding multi-line strings. This library already decodes them fine.

As an aside, toml 0.4.0 has a syntax for multi line strings with the triple quotes that I think would map nicely to go's raw strings

Raw string literals are purely syntactic in Go. You're already free to write strings in your code using whatever flavor of string literal is most convenient.

@srinathh
Copy link
Author

srinathh commented Mar 3, 2016

Well I'd submitted this a while back because I ran into a use case where i needed long strings in the toml file to be readable by humans as well as machines. I usee my patched version while waiting for code review.

JSON/yaml do a terrible job at readability which was the point behind toml development. Since mulit line strings are in the spec and improve readability, I'd love to have this in the core code.

ebrady pushed a commit to dvln/toml that referenced this pull request Oct 24, 2016
* Better logging for parser tests

* Add spew to tests deps list
@igungor
Copy link

igungor commented Jan 21, 2019

I'd like to see this feature in this package. Multiline strings are already part of the spec, also there is no other package that supports encoding strings as multi-line.

@arp242
Copy link
Collaborator

arp242 commented Jun 21, 2021

I'll close this for now as I want to think of a better and more comprehensive solution for this rather than just "slap a struct tag on it". For example, if you decode a TOML multi-line string and encode it again then it should probably get encoded in the same way (i.e. as a multi-line string) without the need to add struct tags all over the place (although you should be able to override it as well).

I have some ideas for a better solution, but I need to prototype a few things to see what does and doesn't work. And before I take this on I want to fix some other things, like supporting all of TOML 1.0, fixing a few remaining bugs, and improving the errors a wee bit.

I absolutely want to add this feature on the short term (i.e. weeks, not years), just need to think of the best way to do it. You can track #64 for updates.

@arp242 arp242 closed this Jun 21, 2021
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.

None yet

6 participants