-
Notifications
You must be signed in to change notification settings - Fork 30
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
add args to conversion to be returnable as json #1512
base: main
Are you sure you want to change the base?
Conversation
Quality Gate passedIssues Measures |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1512 +/- ##
==========================================
+ Coverage 76.40% 76.42% +0.02%
==========================================
Files 618 618
Lines 46858 46822 -36
Branches 845 845
==========================================
- Hits 35803 35786 -17
+ Misses 10959 10940 -19
Partials 96 96
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -67,6 +70,7 @@ def as_json(*_a) | |||
result['converted_type'] = @converted_type if @converted_type | |||
result['converted_bit_size'] = @converted_bit_size if @converted_bit_size | |||
result['converted_array_size'] = @converted_array_size if @converted_array_size | |||
result['args'] = JSON.generate(@args) if @args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as_json shouldn't call JSON.generate. The goal is return objects that are ready to be converted to JSON, but not return an actual JSON string yet. Change to "= @params.as_json(*a)" and remove underscore from the existing _a
@@ -61,4 +63,6 @@ def as_json(self): | |||
result["converted_bit_size"] = self.converted_bit_size | |||
if self.converted_array_size is not None: | |||
result["converted_array_size"] = self.converted_array_size | |||
if self.args is not None: | |||
result["args"] = json.dumps(self.args) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same comment as in Ruby.
@@ -17,6 +17,7 @@ def __init__(self): | |||
# a string, integer, float, or array of values. | |||
# @param packet [Packet] The packet object where the conversion is defined | |||
# @param buffer [String] The raw packet buffer | |||
# @param args [Array] The arguments to return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
args is not an parameter to call. remove.
@@ -30,12 +30,15 @@ class Conversion | |||
attr_reader :converted_bit_size | |||
# @return [Integer] The size in bits of the converted array value | |||
attr_reader :converted_array_size | |||
# @return [] The Array of arguments passed in | |||
attr_reader :args |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably rename to params. If you look at the as_json method in a lot of our existing conversion classes they already are reporting their arguments as 'params'
@@ -12,6 +12,8 @@ def initialize | |||
# Size of the converted type in bits | |||
# Use 0 for :STRING or :BLOCK where the size can be variable | |||
@converted_bit_size = 0 | |||
# return the arguments used | |||
@args = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should default to nil, like in the base class.
Quality Gate passedIssues Measures |
Closes Issue #698