Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
DSA: Inject default in bid requests if not present #3540
DSA: Inject default in bid requests if not present #3540
Changes from 16 commits
23eca73
9cddf2b
e0c174e
a424d16
3d0a754
ea58098
2da4088
1bc45e6
3828209
d2ec47e
94857a3
e202460
c6c0d6f
2ee9652
8c29cb1
ff93f52
5729ab7
bdfca44
f216c82
bcf23f3
59309eb
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 don't know if this is a stronger version of the function, but here's a version where we combine all of the conditions that result in us returning nil, rather than breaking them all up:
What do you think?
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 think this is really a matter of preference. I see your perspective that there are multiple conditionals that result in the same return value, though my tendency is to break complex conditionals into logical groupings because I think it is easier to digest and instills confidence in the intended behavior. I also think it is easier to scan code vertically. In that case, at a minimum the first two conditionals could be grouped as such:
if req == nil || getReqDSA(req) != nil {
.@SyntaxNode do you feel strongly either way here?
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.
Yeah I'm thinking your latest update looks great
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.
There is some danger for memory corruption in pointing the request to using the same
DefaultUnpacked
instance for all requests. However, we also have the convention of "clone + set" when making changes to the request which likely mitigates this issue.