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

add Marshaler interface #137

Closed
wants to merge 1 commit into from
Closed

Conversation

kovetskiy
Copy link

Hi @BurntSushi, I've noticed that there is no nice way to handle Marshal/Encode operation unless implementing encoding.TextMarshaler interface, which can't be used if struct should be also marshalable to JSON (json libraries also looks for encoding.TextMarshaler),

so I've added Marshaler interface with following signature:

type Marshaler interface {
    MarshalTOML() ([]byte, error)
}

Also I've added various tests for this feautre.

@kovetskiy
Copy link
Author

@BurntSushi ping

@@ -114,6 +120,9 @@ func (enc *Encoder) encode(key Key, rv reflect.Value) {
case time.Time, TextMarshaler:
enc.keyEqElement(key, rv)
return

case Marshaler:
Copy link
Collaborator

Choose a reason for hiding this comment

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

If a user-defined type is a Marshaler and a TextMarshaler, the former should take precedence.

@kovetskiy
Copy link
Author

@cespare I've force pushed changes with Marshaler precedence.

@kovetskiy
Copy link
Author

@BurntSushi @cespare ping

@kovetskiy
Copy link
Author

@BurntSushi @cespare any reaction?

@cespare
Copy link
Collaborator

cespare commented Jun 9, 2016

Fixes #76
Related to #63

@cespare
Copy link
Collaborator

cespare commented Jun 9, 2016

@kovetskiy sorry, been busy. I'll try to take a look this weekend.

@kovetskiy
Copy link
Author

@cespare @BurntSushi ping

1 similar comment
@kovetskiy
Copy link
Author

@cespare @BurntSushi ping

@tucnak
Copy link

tucnak commented Jul 7, 2016

jeez @kovetskiy it must hurt lol

@kovetskiy
Copy link
Author

@tucnak yeah, such maintainers

@@ -537,6 +551,21 @@ func encPanic(err error) {
panic(tomlEncodeError{err})
}

func marshalDecode(rv reflect.Value) reflect.Value {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You're already calling rv.Interface().(Marshaler) in the caller; pass in a Marshaler rather than a reflect.Value and avoid the duplicate work.

@cespare
Copy link
Collaborator

cespare commented Jul 7, 2016

I was hoping that adding Marshaler could be a solution for #64, #75, and other cases where there's no way to output a particular encoded value.

For example, I was thinking that if you have a MarshalTOML method like

func (T) MarshalTOML() ([]byte, error) {
    return []byte(`# hello
a = """
1
2
3"""
b = '\n'`), nil
}

then that would yield a TOML encoding with a comment, a multi-line string, and a literal string:

# hello
a = """
1
2
3"""
b = '\n'

but since your approach is to decode and then encode the value, we instead get

a = "1\n2\n3"
b = "\\n"

I agree that we need to decode the value to check that it's valid TOML, but I think we should emit the original encoding. WDYT?

@langston-barrett
Copy link

@kovetskiy Any plans to address the comments here?

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

4 participants