-
Notifications
You must be signed in to change notification settings - Fork 96
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
refactor(JSONObject): extend JSONObject.Delete() to delete multiple entries #1729
base: main
Are you sure you want to change the base?
Conversation
|
"github.com/dynatrace/dynatrace-configuration-as-code/v2/pkg/config/parameter/value" | ||
) | ||
|
||
type JSONObject map[string]any | ||
|
||
// NewJSONObject is a function that creates a JSONObject from a raw JSON byte slice. | ||
func NewJSONObject(raw []byte) (JSONObject, error) { | ||
func NewJSONObject(raw json.RawMessage) (JSONObject, error) { |
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.
I am not sure why you changed this here to json.RawMessage
.
Without this change, I would've approved the PR immediately, as it's straightforward enough.
Since json.RawMessage
is also not a type alias but a real type, I am really not sure if we should introduce it everywhere 🤔
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.
I agree - is there a need for this change? I believe the reason for json.RawMessage
is be able to implement Marshaler
and Unmarshaler
.
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.
The usage of json.RawMessage
emphasizes that the input should be JSON.
Could you explain the problems that defining an argument as json.RawMessage
causes?
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.
Overall this is stylistic:
-
The current version,
r.Delete(bucketName) r.Delete(status) r.Delete(version) r.Delete(updatable)
is easier to read.
-
The proposed version
r.Delete(bucketName, status, version, updatable)
is less verbose.
This PR brings small improvement which enables deleting multiple entries at once.
Instead current
it is possible to: