removed pbtypes.Timestamp

[sofiia-tesliuk]

Removed custom pbtypes.Timestamp and replaced it with bytes.
It is done to remove extra dependency.

[codecov]

Codecov Report

Merging #51 into master will increase coverage by 47.67%.
The diff coverage is 77.77%.

@@             Coverage Diff             @@
##           master      #51       +/-   ##
===========================================
+ Coverage   28.31%   75.99%   +47.67%     
===========================================
  Files           5        4        -1     
  Lines        1155      454      -701     
===========================================
+ Hits          327      345       +18     
+ Misses        783       63      -720     
- Partials       45       46        +1     

Impacted Files	Coverage Δ
diff/diff.go	86.48% <ø> (ø)
diff/parse.go	82.20% <75.00%> (+1.16%)	⬆️
diff/print.go	42.10% <100.00%> (-2.64%)	⬇️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f935979...c3d835f. Read the comment docs.

[schachmat]

Left some comments.

[keegancsmith]

@sqs mind taking a look at this?

@sofiia-tesliuk The direct use of bytes looks a bit funky, but I'm not that familiar with whats better in the land of proto. Maybe providing a little helper around time.MarshalByte in the test cases would make this look a bit less weird.

[schachmat]

@sofiia-tesliuk The direct use of bytes looks a bit funky, but I'm not that familiar with whats better in the land of proto. Maybe providing a little helper around time.MarshalByte in the test cases would make this look a bit less weird.

Using the built-in MarshalByte functions from the standard library avoids the additional dependency and from a protobuf side this is totally supported. Theoretically we could also format the timestamp to a string and then parse it again, but that would take longer, be more brittle (as we'd have to use a time format) and I don't think it'll give us much benefit as the protobuf is only meant for temporary data storage and not really for human consumption (iiuc).

I agree that the literal []byte{…} slices in the test file are not very nice. We can probably improve that by declaring a single []byte variable in package scope and assign the correct timestamp (it seems all tests use the same timestamp) to it in init() by calling the marshal method. On error we can even panic (similar to regexp.MustCompile). In the test case definitions we can then just use this variable instead of the literal byte slice.

[sqs]

I’m not aware of anyone using the .proto in this repo. Since we’d be breaking backcompat anyway, I think this change should also remove the whole .proto and just use Go structs as the definitions for types. Are you open to expanding this diff to do that?

[sofiia-tesliuk]

Looks like travis-ci trying to install go version 1.12, while in go.mod it is mentioned 1.14.
Probably package errors was introduced in later version.

[dmitshur]

Probably package errors was introduced in later version.

Yes, support for error wrapping in the standard errors package is new to Go 1.13. You can use the golang.org/x/xerrors package instead if it's a goal to support Go 1.12 and older, but otherwise it should be a matter of selecting a newer Go version in travis.yml.

[schachmat]

Awesome. PR looks good.
Left one question about a codecov report which seems to be wrong.

[schachmat]

isn't this line exactly the one we're testing for in diff_test.go? Why does Codecov say that this line is not covered by tests?

[keegancsmith]

Requesting changes due to the change from *ParseError to ParseError? Is there a reason for that change?

The reason this could be bad is any checks in client code like if perr, ok := (err).(*diff.ParseError) will now have ok never be true. While in the past it would of been. This is made worse since this won't be picked up by the compiler, is purely at runtime. Most code out in the wild hasn't migrated to things like errors.Is/etc, so we need to support that use case.

[keegancsmith]

FYI thank you so much for this change. I really like it. Just have the one concern in the above review. Otherwise LGTM.

[keegancsmith]

For `&time.Unix(1255273940, 0)` receiving an error: `cannot take the address of time.Unix(1255273940, 0)`.

you can add a tiny helper func unix(sec int64) *time.Time { t := time.Unix(sec, 0) return &t }

[keegancsmith]

nice stuff!

[sofiia-tesliuk]

Could you please also update release version to include new changes?

[keegancsmith]

@sofiia-tesliuk thanks and published v0.6.0

[sofiia-tesliuk]

@keegancsmith Great! thank you so much! :)