-
Notifications
You must be signed in to change notification settings - Fork 47
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
diff: fix time format #17
Conversation
It seems that the fractional seconds part is optional, so change the format to accept seconds. Here's a sample diff output by the bzr command which fails to parse currently. I started adding a test, but it involves more time than I have to spend right now, as the tests rely on reflect.DeepEquals working on time.Time instances, which won't work in general due to monotonic time. I'd suggest using https://godoc.org/github.com/google/go-cmp/cmp#Diff instead. === modified file 'encode.go' --- encode.go 2011-11-24 19:47:20 +0000 +++ encode.go 2011-11-28 13:24:31 +0000 @@ -250,8 +250,7 @@ e.emitScalar("null", "", "", C.YAML_PLAIN_SCALAR_STYLE) } -func (e *encoder) emitScalar(value, anchor, tag string, - style C.yaml_scalar_style_t) { +func (e *encoder) emitScalar(value, anchor, tag string, style C.yaml_scalar_style_t) { var canchor, ctag, cvalue *C.yaml_char_t var cimplicit C.int var free func() === modified file 'encode_test.go' --- encode_test.go 2011-11-24 19:47:20 +0000 +++ encode_test.go 2011-11-28 13:24:39 +0000 @@ -76,6 +76,10 @@ A int "a,omitempty" B int "b,omitempty" }{0, 0}}, + {"{}\n", &struct { + A *struct{ X int } "a,omitempty" + B int "b,omitempty" + }{nil, 0}}, // Flow flag {"a: [1, 2]\n", &struct { === modified file 'goyaml.go' --- goyaml.go 2011-11-24 19:47:20 +0000 +++ goyaml.go 2011-11-28 13:24:39 +0000 @@ -256,7 +256,7 @@ switch v.Kind() { case reflect.String: return len(v.String()) == 0 - case reflect.Interface: + case reflect.Interface, reflect.Ptr: return v.IsNil() case reflect.Slice: return v.Len() == 0
Thanks! I’ll look into the test probably on Saturday. If anyone else happens to see this PR and wants to get some karma, test patches are happily accepted. |
Edit: Never mind, I accidentally had your change applied when testing. It returns an error without your change:
The Line 270 in 748a069
Line 74 in 748a069
Changing it affects how diffs are printed, which doesn't seem to be what you intended with this PR, and causes the tests to fail. |
I think the correct fix is to have 2 constants, one as the parse layout, and the other as the print layout. They're no longer going to be the same. Adding a basic test should be easy, just add a new file to |
Also, according to package
So the parse layout string I'll send a PR that resolves this soon. |
Sent PR #18 that should resolve this. It resolves the failing tests, fixes the parsing behavior when header timestamps don't have fractional seconds (as allowed by https://www.gnu.org/software/diffutils/manual/html_node/Detailed-Unified.html; see "The fractional seconds are omitted on hosts that do not support fractional timestamps."), and adds a test for it. |
I'm not entirely convinced. This will cause all output to contain the full nanosecond precision decimals, but often we don't want that. For example, this change means that reading the example diff output above and printing it will change the output - if you add Given that the internal representation loses information about the original precision of the time stamp, I'd suggest that the BTW I'm wondering if that test data (with the all the trailing zeros) was produced by an actual tool or was constructed specifically for the tests? |
I agree. However, I would describe that as a low impact, low priority optional enhancement. It can be done, but it'll require making changes to tests or testdata so they can continue to pass. It seems the value is low because most unified diffs I see these days have fractional seconds.
I'm not sure. The more recent testdata files that I added came from latest versions of |
It seems that the fractional seconds part is optional, so change the format to accept whole seconds too.
Here's a sample diff output by the bzr command which fails to parse currently.
I started adding a test, but it involves more time than I have to spend right now, as the tests rely on reflect.DeepEqual working on time.Time instances, which won't work in general due to monotonic time. I'd suggest using https://godoc.org/github.com/google/go-cmp/cmp#Diff instead.