Skip to content

Conversation

frrist
Copy link
Member

@frrist frrist commented Jan 25, 2022

fixes #819

@frrist frrist requested a review from iand January 25, 2022 23:45
@frrist frrist force-pushed the frrist/msg-params-marshal branch 2 times, most recently from 2aefe2c to 7c579a0 Compare January 26, 2022 00:07
@codecov-commenter
Copy link

codecov-commenter commented Jan 26, 2022

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 31.8%. Comparing base (a473ac6) to head (93aae7e).
Report is 295 commits behind head on master.

Additional details and impacted files
@@          Coverage Diff           @@
##           master    #821   +/-   ##
======================================
  Coverage    31.8%   31.8%           
======================================
  Files          39      39           
  Lines        3777    3777           
======================================
  Hits         1202    1202           
  Misses       2429    2429           
  Partials      146     146           
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@frrist frrist force-pushed the frrist/msg-params-marshal branch from 7c579a0 to 0e110db Compare January 26, 2022 00:20
@frrist frrist mentioned this pull request Jan 26, 2022
@frrist frrist force-pushed the frrist/msg-params-marshal branch from 0e110db to 16fa758 Compare January 26, 2022 00:30
@frrist frrist changed the base branch from master to frrist/update-go January 26, 2022 00:56
@frrist frrist force-pushed the frrist/msg-params-marshal branch from 16fa758 to 4da66a7 Compare January 26, 2022 01:28
Copy link
Contributor

@iand iand left a comment

Choose a reason for hiding this comment

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

While I think this would work it's feels like it's more complex than it needs to be.

How about this alternate approach. Assuming that all message params are simple structs then you could use reflection to convert the param struct to an equivalent map (like is done on line 380 in this PR). If the value of a param field is a bitfield then you wrap it in a struct with its own custom marshaler. Then you just marshal the map.

This approach relies on message params being simple structs without nested maps of other structs, which is valid as far as I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be a lot clearer if you made this a first class function:

type marshaler func(interface{}) ([]byte, error)

func MarshalWithOverrides(v interface{}, overrides map[reflect.Type]marshaler) []byte, error {
  ...
}

The function then uses paramWraperType as an internal implementation detail.

The marshaler returns an error so you can replace the panics with error returns in your bitfield marshaler.

Then you can call it like this:

var bitfieldCountMarshaler = func(v interface{}) ([]byte, error) {
   ...
}

b, err := MarshalWithOverrides(b, map[reflect.Type]marshaler{
   reflect.TypeOf(bitfield.BitField{}): bitfieldCountMarshaler,
})

@frrist frrist force-pushed the frrist/msg-params-marshal branch 4 times, most recently from ef9fa60 to 8608a12 Compare January 31, 2022 21:46
Base automatically changed from frrist/update-go to master February 1, 2022 16:18
@frrist frrist force-pushed the frrist/msg-params-marshal branch from 8608a12 to 93aae7e Compare February 1, 2022 16:40
@frrist frrist merged commit 75d47ee into master Feb 1, 2022
@frrist frrist deleted the frrist/msg-params-marshal branch February 1, 2022 16:49
var ret = struct {
Count uint64
RLE []uint64
}{}
Copy link
Contributor

Choose a reason for hiding this comment

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

Given:

Old:
{"Recoveries": [{"Sectors": {"rle": "[10816,31,1407,1,2,23,1,4,1,1,1405,1,3,26,1,1,1,1,521,1,3,915,58,1,2,1343]", "_type": "bitfield", "elemcount": "2349"}, "Deadline": 6, "Partition": 0}]}

I would have made this struct match the existing schema:

var ret = struct {
  Count uint64 `json:elemcount`
  RLE []uint64 `json:rle`
  Type string `json:_type`
}{ Type: "bitfield" }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Message Parse Param Changes
4 participants