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
Add support for checksum-based interceptors #72814
Add support for checksum-based interceptors #72814
Changes from 8 commits
3f1a038
53e0548
f12ce01
db13fb5
2cb8bfd
355728a
2394377
4b147f7
231c190
1056037
ac01bc4
282bff3
5f85fcb
8ac441d
9763c4f
5bef2bc
499fd18
e1f9273
13ece53
b2b30c5
00e90be
2d65d38
e8aed9f
9230536
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.
Was there a discussion of embedding the version number directly in the opaque data string? Maybe
<version>-<data>
?The version number doesn't seem especially useful when I look at an attribute like
[InterceptsLocation(1, "yPU5+1/pMuRHlz+XbnIQwQYAAAARAAAAUHJvZ3JhbS5jcw==")]
Also, consider adding an example of location encoding in the doc to show what it looks like.
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.
It was thought that this gives the ability to change the precise encoding used in the data string, e.g. maybe in the future we want something different than base64, splitting out the version gives us the ability to express that.
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.
Looked again and I think I better understand the suggestion, which was to have a prefix that isn't necessarily encoded the same way. Yes, it is expressible either way. The idea of combining them together for the "opaque encoding" approach didn't come up in API review.
The only reason I can think of to keep it this way is to separate parts that we think should be user-readable from those that are not, when it comes to diagnosing version incompatibility issues (which would arise from compiling later interceptors code in an earlier compiler.)
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.
What does it mean to be "intercepted syntactically"? Perhaps drop "syntactically" if it is not adding anything. #Closed
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.
c.P()
whereP
is defined asAction P { get; }
can be intercepted syntactically but an error will occur when you actually try to do it. Basically, the comment is meant to caution that even if you get a non-null location out, intercepting may still fail.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.
Do we have a test for the case of a
null
return because the location is syntactically considered to be not-interceptable? #Closed