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

fix: Update immutable package #1290

Merged
merged 2 commits into from
Apr 6, 2023

Conversation

islamaliev
Copy link
Contributor

@islamaliev islamaliev commented Apr 5, 2023

New version adds custom un/marshaling

Relevant issue(s)

Resolves #1289

Description

Update immutable package that added custom MarshalJSON and UnmarshalJSON method to immutable.Option

Tasks

  • I made sure the code is well commented, particularly hard-to-understand areas.
  • I made sure the repository-held documentation is changed accordingly.
  • I made sure the pull request title adheres to the conventional commit style (the subset used in the project can be found in tools/configs/chglog/config.yml).
  • I made sure to discuss its limitations such as threats to validity, vulnerability to mistake and misuse, robustness to invalidation of assumptions, resource requirements, ...

How has this been tested?

Integration tests and manual tests.

Specify the platform(s) on which this was tested:

  • MacOS

@islamaliev islamaliev added the bug Something isn't working label Apr 5, 2023
@islamaliev islamaliev added this to the DefraDB v0.5 milestone Apr 5, 2023
@islamaliev islamaliev requested a review from jsimnz April 5, 2023 10:21
@islamaliev islamaliev self-assigned this Apr 5, 2023
@islamaliev islamaliev linked an issue Apr 5, 2023 that may be closed by this pull request
@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

go.sum Outdated
Comment on lines 911 to 912
github.com/sourcenetwork/immutable v0.2.1 h1:0SAnoiGm1XQG+xiG4gK4nmLFqTMRnQ5Y1N4WHL7vQtE=
github.com/sourcenetwork/immutable v0.2.1/go.mod h1:4jpxObkIQw8pvlIRm4ndZqf3pH9ZjYEw/UYI6GZDJww=
Copy link
Member

@shahzadlone shahzadlone Apr 5, 2023

Choose a reason for hiding this comment

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

todo: These two lines shouldn't exist (ones with v0.2.1). Run make tidy it should fix that by removing these.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch

@codecov
Copy link

codecov bot commented Apr 5, 2023

Codecov Report

Merging #1290 (bfd223b) into develop (e1d7545) will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1290      +/-   ##
===========================================
+ Coverage    70.40%   70.47%   +0.07%     
===========================================
  Files          184      184              
  Lines        17825    17825              
===========================================
+ Hits         12549    12563      +14     
+ Misses        4320     4310      -10     
+ Partials       956      952       -4     

see 4 files with indirect coverage changes

@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

Copy link
Collaborator

@fredcarle fredcarle left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for doing the change.

@islamaliev islamaliev force-pushed the islam/fix/I1289-immutable-with-custom-marshal branch from be12b69 to 58e9b30 Compare April 5, 2023 19:22
@source-devs
Copy link

Benchmark Results

Summary

  • 0 Benchmarks successfully compared.
  • 0 Benchmarks were ✅ Better.
  • 0 Benchmarks were ❌ Worse .
  • 0 Benchmarks were ✨ Unchanged.
✅ See Better Results...
time/opdelta
 
❌ See Worse Results...
time/opdelta
 
✨ See Unchanged Results...
time/opdelta
 
🐋 See Full Results...

@jsimnz jsimnz added the action/no-benchmark Skips the action that runs the benchmark. label Apr 5, 2023
New version adds custom un/marshaling
@islamaliev islamaliev force-pushed the islam/fix/I1289-immutable-with-custom-marshal branch from 58e9b30 to bfd223b Compare April 6, 2023 06:11
@islamaliev islamaliev merged commit 7356595 into develop Apr 6, 2023
@islamaliev islamaliev deleted the islam/fix/I1289-immutable-with-custom-marshal branch April 6, 2023 06:22
shahzadlone pushed a commit that referenced this pull request Apr 13, 2023
New version adds custom un/marshaling
shahzadlone pushed a commit to shahzadlone/defradb that referenced this pull request Feb 23, 2024
New version adds custom un/marshaling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
action/no-benchmark Skips the action that runs the benchmark. bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

immutable.Option has wrong marshal/unmarshal behaviour
5 participants