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.

Sign up for GitHub

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

Jump to bottom

Update ugorji/go/codec to latest version #830

Closed
wants to merge 7 commits into from
Closed

Update ugorji/go/codec to latest version #830

wants to merge 7 commits into from

Conversation

zhsj
Copy link
Contributor

@zhsj zhsj commented Apr 13, 2019

Requirements

All new code should be covered with tests, documentation should be updated. CI should pass.

Description of the Change

The vendored ugorji/go/codec is rather old...

Checklist

  • unit-test added (if change is algorithm)
  • functional test added/updated (if change is functional)
  • man page updated (if applicable)
  • bash completion updated (if applicable)
  • documentation updated
  • author name in AUTHORS

@zhsj
Add codec tag to fields which are ignored in new codec package
3070658 
github.com/ugorji/go/codec 1.1.4 ignores field with json:"-" tag
@zhsj
Fix time.Time msgpack decoding backwards compatibility
951704a 
See ugorji/go#269
@zhsj
Update AUTHORS
99a293b
@zhsj
Fix over-indented error in cleanup.py
98afdec 
This fix lint error:

. system/env/bin/activate && flake8 --max-line-length=200 --exclude=system/env/ system/
system/t08_db/cleanup.py:112:13: E117 over-indented
@zhsj zhsj force-pushed the update-codec branch 5 times, most recently from 9b3b626 to 72c2f53 Compare April 13, 2019 17:27
@codecov
Copy link

codecov bot commented Apr 13, 2019 •
edited
Loading

Codecov Report

Merging #830 into master will decrease coverage by 0.15%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #830      +/-   ##
==========================================
- Coverage   64.23%   64.07%   -0.16%     
==========================================
  Files          51       51              
  Lines        6514     6530      +16     
==========================================
  Hits         4184     4184              
- Misses       1825     1841      +16     
  Partials      505      505
Impacted Files Coverage Δ
deb/local.go 84.48% <ø> (ø) ⬆️
deb/remote.go 61.79% <0%> (-1.1%) ⬇️
deb/snapshot.go 73.42% <0%> (-2.75%) ⬇️

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 e2d6a53...f7ddb8e. Read the comment docs.

@zhsj
Copy link
Contributor Author

zhsj commented Apr 13, 2019 •
edited
Loading

Finally fixed the testsuites...

It's broken by that http://mirror.yandex.ru/debian/dists/wheezy/ and others are 404.

The last 3 commits fixed the failure not caused by this change. Would you like to open another PR or keep them in this PR?

As for the test coverage, no sure how to cover the else clause for old DB backwards compatibility.

@smira
Copy link
Contributor

smira commented Jul 5, 2019

thanks, I have the system tests fixed, but I really like your approach with updating that unfortunately not backwards compatible codec

@smira smira added the 1.5.0 label Jul 10, 2019
@smira smira mentioned this pull request Jul 12, 2019
Merged
@smira
Copy link
Contributor

smira commented Jul 12, 2019

@zhsj thanks for the awesome PR, I rebased that against master in #855

@smira smira closed this Jul 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@zhsj @smira