Snippet production #2629
Snippet production #2629
Conversation
michaelcfanning
requested review from
EasyRhinoMSFT,
marmegh and
cfaucon
as code owners
|
Need a review asap on this. @suvamM @EasyRhinoMSFT @shaopeng-gh @yongyan-gh @HulonJenkins |
| @@ -164,6 +164,11 @@ public Region ConstructMultilineContextSnippet(Region inputRegion, Uri uri, stri | |||
| // Generating full inputRegion to prevent issues. | |||
| Region originalRegion = this.PopulateTextRegionProperties(inputRegion, uri, populateSnippet: true, fileText); | |||
|
|
|||
| if (originalRegion.CharLength >= BIGSNIPPETLENGTH) | |||
There was a problem hiding this comment.
why? if the size of the original region is as big as our bigsnippet maximum, why would attempt to generate it? what am i missing?
There was a problem hiding this comment.
I was verifying that the max length is 511, since the >= operator excludes 512. This seems to be an off-by-one considering the statement that snippets are "restricted to 512 bytes in size"
| @@ -745,8 +740,7 @@ public void FileRegionsCache_PopulatesWithOneLine_IncreasingToTheRight() | |||
| region = fileRegionsCache.PopulateTextRegionProperties(region, uri, true, content); | |||
| Region multilineRegion = fileRegionsCache.ConstructMultilineContextSnippet(region, uri); | |||
|
|
|||
| // CharLength + 128 to the right = 428 characters | |||
| multilineRegion.CharLength.Should().Be(300 + 128); | |||
| multilineRegion.CharLength.Should().Be(512); | |||
There was a problem hiding this comment.
Curious, with the bug regarding > above, how does this test pass?
There was a problem hiding this comment.
I don't understand the bug that you see. big snippet is defined as 512.
|
Just wanted to comment on the fantastic PR description: brief and informative. Thanks @michaelcfanning |
| @@ -745,8 +740,7 @@ public void FileRegionsCache_PopulatesWithOneLine_IncreasingToTheRight() | |||
| region = fileRegionsCache.PopulateTextRegionProperties(region, uri, true, content); | |||
| Region multilineRegion = fileRegionsCache.ConstructMultilineContextSnippet(region, uri); | |||
|
|
|||
| // CharLength + 128 to the right = 428 characters | |||
| multilineRegion.CharLength.Should().Be(300 + 128); | |||
| multilineRegion.CharLength.Should().Be(512); | |||
There was a problem hiding this comment.
And shouldn't that be a constant? #Resolved
There was a problem hiding this comment.
+1 to Craig's comment. We should have an Assume section in the test defining the constants.
There was a problem hiding this comment.
We now pick it up from the product, good call.
|
:) In reply to: 1448659817 |
We now create context region snippets that are restricted to 512 bytes in size. In the event that the original snippet is larger than this, we simply recapitulate that snippet in the log data (i.e., the context region is identical).