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

Parse fields that implement TextUnmarshaller interface #148

Merged
6 commits merged into from
Jan 4, 2017
Merged

Conversation

ghost
Copy link

@ghost ghost commented Dec 31, 2016

Right now we can parse only bools, ints and strings. This change allows to convert strings to types that implement encoding.TextUnmarshaller interface, e.g. time.Time or math/big.Int

@mention-bot
Copy link

@alsamylkin, thanks for your PR! By analyzing the history of the files in this pull request, we identified @shawnburke, @anuptalwalkar and @glibsm to be potential reviewers.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.935% when pulling dc134f1 on unmarshalText into cbef20b on master.

@glibsm
Copy link
Collaborator

glibsm commented Dec 31, 2016

Could you run the bench and paste the results in the PR description?

case *bool:
return strconv.ParseBool(v)
case encoding.TextUnmarshaler:
err := target.(encoding.TextUnmarshaler).UnmarshalText([]byte(v))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is []byte(v) always going to be happy?

Copy link
Author

Choose a reason for hiding this comment

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

From my understanding string is just a readonly byte slice, so yes, it will be always happy.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.935% when pulling 62aa926 on unmarshalText into cbef20b on master.

switch targetType.Name() {
case "int":
target := reflect.New(targetType).Interface()
switch target.(type) {
Copy link
Contributor

Choose a reason for hiding this comment

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

do a colon equals assignment here like line 338 so you don't have to re-cast on 348

@@ -192,5 +192,19 @@
// Note that any fields you wish to deserialize into must be exported, just like
// json.Unmarshal and friends.
//
// Benchmarks
Copy link
Author

Choose a reason for hiding this comment

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

Master branch:
BenchmarkYAMLCreateSingleFile-8 50000 31637 ns/op 10832 B/op 121 allocs/op
BenchmarkYAMLCreateMultiFile-8 30000 52003 ns/op 19840 B/op 207 allocs/op
BenchmarkYAMLSimpleGetLevel1-8 50000000 27.0 ns/op 0 B/op 0 allocs/op
BenchmarkYAMLSimpleGetLevel3-8 50000000 27.5 ns/op 0 B/op 0 allocs/op
BenchmarkYAMLSimpleGetLevel7-8 50000000 26.8 ns/op 0 B/op 0 allocs/op
BenchmarkYAMLPopulateStruct-8 2000000 879 ns/op 192 B/op 10 allocs/op
BenchmarkYAMLPopulateStructNested-8 500000 2744 ns/op 632 B/op 34 allocs/op
BenchmarkYAMLPopulateStructNestedMultipleFiles-8 500000 3684 ns/op 816 B/op 45 allocs/op

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.935% when pulling 2f7c257 on unmarshalText into cbef20b on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.1%) to 91.935% when pulling 0809afb on unmarshalText into cbef20b on master.

Copy link
Contributor

@anuptalwalkar anuptalwalkar left a comment

Choose a reason for hiding this comment

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

lgtm

- hero: LaunchpadMcQuack
- hero: LaunchpadMcQuack
- hero: Scrooge
- hero: Scrooge
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: its complaining about new line here.

@coveralls
Copy link

coveralls commented Jan 4, 2017

Coverage Status

Coverage increased (+0.1%) to 91.935% when pulling 0cddefd on unmarshalText into cbef20b on master.

@ghost ghost merged commit f82f109 into master Jan 4, 2017
@ghost ghost deleted the unmarshalText branch January 4, 2017 02:25
@ascandella
Copy link
Contributor

ascandella commented Jan 4, 2017 via email

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

5 participants