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

Removed write quote in marshal to allow write other types than strings #344

Merged

Conversation

lucasbutn
Copy link

Main issue was that in enconde.go Line 215, the marshaled result was wrote with quotes, making imposible to write other values than strings.

Now the implementer of marshal, is responsible to write the quotes if required, as it happens in other marshalers if we compare it to JSON marshal, but it gives more flexibility.

The full issue is explained here: #341

An easy way to verify the issue is change enconde.go Line 215 to its previous state an run the test

@arp242
Copy link
Collaborator

arp242 commented Feb 11, 2022

The existing TestEncodeTOMLMarshaler seems to fail now; I think fixing that is just a matter of mucking about with quotes in the MarshalTOML() methods, since it doesn't add any now. I fixed the CI in master, so if you rebase that should work.

But other than that, looks good.

@lucasbutn lucasbutn force-pushed the hotfix-341-marshaler-shouldnot-writequoted branch from 62ff2d2 to 247a92b Compare February 11, 2022 17:06
@lucasbutn
Copy link
Author

The existing TestEncodeTOMLMarshaler seems to fail now; I think fixing that is just a matter of mucking about with quotes in the MarshalTOML() methods, since it doesn't add any now. I fixed the CI in master, so if you rebase that should work.

But other than that, looks good.

Sorry, I've neglect the tests. I've fixed them and add extra coverage for Slices and Complex.

@arp242
Copy link
Collaborator

arp242 commented Feb 12, 2022

Not sure why I needed to approve the CI runs again 🤔 Anyway, now the Go 1.13 tests fails with:

Error: ./custom_marshaler_test.go:170:23: undefined: strconv.FormatComplex
Error: ./custom_marshaler_test.go:178:17: undefined: strconv.ParseComplex

Looks like these functions were added in Go 1.15, whereas this library tries to be compatible with Go 1.13 and newer (for the new errors interface); I'm not opposed to bumping that to a newer version, but since it's just for a test I'd rather not since you can get the same effect with some basic string parsing. Bit annoying it's extra work though; sorry about that :-(

@lucasbutn lucasbutn force-pushed the hotfix-341-marshaler-shouldnot-writequoted branch from 247a92b to dec5825 Compare February 16, 2022 15:07
@lucasbutn
Copy link
Author

Not sure why I needed to approve the CI runs again 🤔 Anyway, now the Go 1.13 tests fails with:

Error: ./custom_marshaler_test.go:170:23: undefined: strconv.FormatComplex
Error: ./custom_marshaler_test.go:178:17: undefined: strconv.ParseComplex

Looks like these functions were added in Go 1.15, whereas this library tries to be compatible with Go 1.13 and newer (for the new errors interface); I'm not opposed to bumping that to a newer version, but since it's just for a test I'd rather not since you can get the same effect with some basic string parsing. Bit annoying it's extra work though; sorry about that :-(

I've just removed the complex testing for now. It is no different from any CustomMarshall to a string format annyway. since toml doest'n suppors '(' and ')', or '+' there is no other way to marshal a complex than a string in this case.
Sorry I didn't had any tima before

Thanks. Hope CI runs ok this time

@arp242 arp242 merged commit 551f4a5 into BurntSushi:master Feb 17, 2022
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

2 participants