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

Deprecate SetData method #288

Merged
merged 2 commits into from
Oct 2, 2023
Merged

Deprecate SetData method #288

merged 2 commits into from
Oct 2, 2023

Conversation

mfdeveloper508
Copy link
Contributor

@codecov-commenter
Copy link

codecov-commenter commented Oct 2, 2023

Codecov Report

Attention: 23 lines in your changes are missing coverage. Please review.

Comparison is base (b05481c) 73.95% compared to head (05bccd5) 73.43%.

❗ Current head 05bccd5 differs from pull request most recent head be33dce. Consider uploading reports for the commit be33dce to get more accurate results

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #288      +/-   ##
==========================================
- Coverage   73.95%   73.43%   -0.52%     
==========================================
  Files          43       43              
  Lines        2338     2338              
==========================================
- Hits         1729     1717      -12     
- Misses        378      390      +12     
  Partials      231      231              
Files Coverage Δ
message.go 74.10% <ø> (-0.80%) ⬇️
field/bitmap.go 69.29% <60.00%> (-1.76%) ⬇️
field/binary.go 60.67% <40.00%> (ø)
field/hex.go 66.29% <40.00%> (-2.25%) ⬇️
field/numeric.go 80.89% <40.00%> (-2.25%) ⬇️
field/string.go 80.72% <40.00%> (-2.41%) ⬇️
field/track1.go 69.02% <40.00%> (ø)
field/track2.go 68.22% <40.00%> (ø)
field/track3.go 61.79% <40.00%> (ø)

... and 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mfdeveloper508 mfdeveloper508 changed the title Issue 278 Deprecate SetData method Oct 2, 2023
return nil
}

bin, ok := data.(*Binary)
bin, ok := v.(*Binary)
Copy link
Contributor

Choose a reason for hiding this comment

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

@mfdeveloper508 here and in the rest of the fields, we don't need f.data anymore. Before, it was used in SetData and was a pointer that was used when we set and get "data". With Marhsal and Unmarshl - we don't need to store the pointer to the field type as we use field value directly.

        f.data = bin // <- the field can be removed
	if bin.value != nil {
		f.value = bin.value
	}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#289
right fixed in next pr

return nil
}

bmap, ok := data.(*Bitmap)
bmap, ok := v.(*Bitmap)
Copy link
Contributor

Choose a reason for hiding this comment

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

In this field, we need data as it's basically value - it stores the value of the field. Please, don't be confused with my previous statement that we should remove data from all fields. When data *String (or pointer to other field type - then we should remove it), but here - data has the same purpose as value in other fields. It just has different name.

@@ -23,7 +23,7 @@ type Field interface {
// Bytes returns binary representation of the field Value
Bytes() ([]byte, error)

// Deprecated. Use Marshal intead.
// Deprecated. Use Marshal instead.
Copy link
Contributor

Choose a reason for hiding this comment

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

Good catch!

return nil
}

str, ok := data.(*Hex)
str, ok := v.(*Hex)
Copy link
Contributor

Choose a reason for hiding this comment

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

below, we can remove f.data

return nil
}

num, ok := data.(*Numeric)
num, ok := v.(*Numeric)
Copy link
Contributor

Choose a reason for hiding this comment

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

here we can remove data as well

packed, err = numeric.Pack()
require.NoError(t, err)
require.Equal(t, " 9876", string(packed))

numeric = NewNumeric(spec)
data := NewNumericValue(0)
numeric.SetData(data)
numeric.Marshal(data)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will fail after we remove data from the Numeric field. The behavior of SetData is slightly different from Marshal. Just test that Marshal sets value and Unmarshal sets value back to the arg.

return nil
}

str, ok := data.(*String)
str, ok := v.(*String)
Copy link
Contributor

Choose a reason for hiding this comment

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

f.data can be removed from the String field

packed, err = str.Pack()
require.NoError(t, err)
require.Equal(t, " hello", string(packed))

str = NewString(spec)
data := NewStringValue("")
str.SetData(data)
str.Marshal(data)
length, err = str.Unpack([]byte(" olleh"))
require.NoError(t, err)
require.Equal(t, 10, length)
require.Equal(t, "olleh", data.Value())
Copy link
Contributor

Choose a reason for hiding this comment

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

This test will also fail as if we remove data it will no longer be used in Unpack (internally SetBytes). Just test Marshal and Unmarshal.

return nil
}

track, ok := data.(*Track1)
track, ok := v.(*Track1)
Copy link
Contributor

Choose a reason for hiding this comment

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

f.data can be removed from the Track1

return nil
}

track, ok := data.(*Track2)
track, ok := v.(*Track2)
Copy link
Contributor

Choose a reason for hiding this comment

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

f.data can be removed from Track2

return nil
}

track, ok := data.(*Track3)
track, ok := v.(*Track3)
Copy link
Contributor

Choose a reason for hiding this comment

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

here we can also remove f.data.

@alovak
Copy link
Contributor

alovak commented Oct 2, 2023

@mfdeveloper508 thanks for breaking down your PR into smaller ones. For this PR the summary of my review is:

  1. we should remove data field from all fields such as Numeric, String, etc. Please note, that in Binary field, it should stay or be renamed into value as it’s named in all fields.
  2. we should update some tests as after we remove data - I believe ~2-3 tests will fail. Just simplify them to keep tests only for Marshal and Unmarshal methods (setting/getting data in/from the passed arg)

Copy link
Contributor

@alovak alovak left a comment

Choose a reason for hiding this comment

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

LGTM!

@alovak alovak merged commit b435156 into master Oct 2, 2023
9 checks passed
@alovak alovak deleted the issue-278 branch October 2, 2023 17:21
@alovak alovak mentioned this pull request Oct 6, 2023
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.

3 participants