Add command to create rascal location from selection #283
Conversation
|
@jurgenvinju can you give this a look? |
|
Cool feature 👍🏼 I'm still on holiday, but I wanted to note that it's not
an accurate calculation of a rascal location, since vs code is utf16
offset/column based, while rascal is utf32.
This line would mess up the offsets:
```
/* 👋🦾 */ println("Hi");
```
…On Mon, 21 Aug 2023, 23:01 Linus Wagner, ***@***.***> wrote:
@jurgenvinju <https://github.com/jurgenvinju> can you give this a look?
—
Reply to this email directly, view it on GitHub
<#283 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABL3E5PLWORZQU4ZQMG4PDXWPEB5ANCNFSM6AAAAAA3Y54DC4>
.
You are receiving this because you are subscribed to this thread.Message
ID: ***@***.***>
|
|
@linuswagner @DavyLandman let's fix the offsets together before we merge the PR. I'd also like to add a feature where a list of tagged locations can be collected in a file (configured by the user). That way we help many users that need to manually get some ground truth about a code corpus. |
|
|
As the person that opens a PR, you can also choose to allow anyone with
commit access to the repo to also have access on your branch.
…On Tue, 22 Aug 2023, 13:50 Linus Wagner, ***@***.***> wrote:
@linuswagner <https://github.com/linuswagner> @DavyLandman
<https://github.com/DavyLandman> let's fix the offsets together before we
merge the PR. I'd also like to add a feature where a list of tagged
locations can be collected in a file (configured by the user). That way we
help many users that need to manually get some ground truth about a code
corpus.
Sure. Keep in mind that I've forked the repo for this PR, so you either
fork the fork and create a PR on mine or we move my commit to a branch here
and close this PR.
—
Reply to this email directly, view it on GitHub
<#283 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AABL3EZTDUSNXRYR2FL3B5DXWSMJDANCNFSM6AAAAAA3Y54DC4>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
There was a problem hiding this comment.
Looks good @linuswagner!
I've added some comments on how to get this to be compatible with more use cases of rascal. Do you have some time to pick that up?
|
@DavyLandman thanks for the feedback. I'll be taking a look on Friday/the weekend and see what I can do. |
awesome 👍🏼 |
|
@DavyLandman took a bit longer than expected, but UTF16 characters now work. If you give me a rough description of the interface you want for UTF8 <-> UTF16 conversion (including where to place it), I can also do the refactoring you further up. |
There was a problem hiding this comment.
Hi @linuswagner, Great work! Thank you for taking the time to pick this up.
With regards to your question. Maybe for now make a util folder? and put the PositionConverter class in there?
|
also @linuswagner the build is failing due to some of es-lint messages, you can run it by |
|
@DavyLandman All done. Can you also please check |
There was a problem hiding this comment.
hi @linuswagner thanks! much imporved 👍🏼
Since you asked me to carefully review the calc functions I did. I think some stuff has to be at least aligned with the documentation of the function. It might be that I misunderstood the code, feel free to point that out and explain what is happening.
| * @returns the column as understood by VS Code. | ||
| */ | ||
| static rascalToVSCodeColumn(td: vscode.TextDocument, line: number, rascalColumn: number): number { | ||
| const fullLine = td.lineAt(line).text; |
There was a problem hiding this comment.
shouldn't this be line - 1? (and maybe named rascalLine?) Or should we document that we assume the caller already has converted the line number to vs code?
| i++; | ||
| } | ||
| } | ||
| return [offset, endOffset]; |
There was a problem hiding this comment.
this has a bug, as the endOffset is not the length. Maybe should it be endOffset - offset ? or we should make it clear it returns 2 positions.
| */ | ||
| static vsCodeToRascalColumn(td: vscode.TextDocument, line: number, columnVSCode: number): number { | ||
| const fullLine = td.lineAt(line).text; | ||
| let lengthRascal = columnVSCode; |
There was a problem hiding this comment.
NP: should it be called columnRascal?
|
|
||
| for (let i = 0; i < columnVSCode - 1; i++) { | ||
| const c = fullLine.charCodeAt(i); | ||
| if (PositionConverter.isHighSurrogate(c) && PositionConverter.isLowSurrogate(fullLine.charCodeAt(i + 1))) { |
There was a problem hiding this comment.
will throw exception if (i + 1) == length.
| const fullLine = td.lineAt(line).text; | ||
| let lengthRascal = columnVSCode; | ||
|
|
||
| for (let i = 0; i < columnVSCode - 1; i++) { |
There was a problem hiding this comment.
we should make sure that columnVSCode is not more than fullLine.length()
| static vsCodeToRascalOffsetLength(td: vscode.TextDocument, offset: number, length: number): [number, number] { | ||
| const fullText = td.getText(); | ||
| const endOffset = offset + length; | ||
| let newEndOffset = endOffset; |
There was a problem hiding this comment.
should be called newLength? and just be newLength = length?
| i++; // the following letter is known to be the low surrogate -> we can skip it | ||
| } | ||
| } | ||
| return [offset, newEndOffset]; |
There was a problem hiding this comment.
should not return 2 positions, but 1 offset, and one length.
| let newEndOffset = endOffset; | ||
| for (let i = 0; i < endOffset - 1; i++) { | ||
| const c = fullText.charCodeAt(i); | ||
| if (PositionConverter.isHighSurrogate(c) && PositionConverter.isLowSurrogate(fullText.charCodeAt(i + 1))) { |
There was a problem hiding this comment.
also, same bug, i+1 might throw exception in case of malformed string.
|
This is a lovely feature to have; I really miss it (since we had it in the Eclipse version). Thanks both for putting your time into this. |
This PR adds a command that allows users to get a Rascal location for their selection put into their clipboard.
If you review this, you should check the following things:
Limitations: