Skip to content
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

FSI send ignore comments #1859 #2049

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jkone27
Copy link

@jkone27 jkone27 commented Nov 10, 2024

fixes issue with comments too long , partial fix for general issue 1859, to avoid FS0193: operation not supported

image

but comments can be safely ignored and skip FSI send, even if very long

image

jkone27 and others added 7 commits November 10, 2024 18:07
Add functionality to skip comments and warn if a line is too long for the interactive session in `src/Components/Fsi.fs`.

- **Skip Comments**: Modify the `sendSelection` function to filter out lines that start with `//` before sending the text to the terminal.

- **Warn for Long Lines**: Add a `detectTerminalSize` function to get the terminal size and modify the `send` function to check if the number of lines exceeds the terminal rows. If it does, show a warning message to the user.

---

For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/jkone27/ionide-vscode-fsharp?shareId=XXXX-XXXX-XXXX-XXXX).
Revert changes in RELEASE_NOTES.md to match master
@jkone27 jkone27 changed the title (fix)1859: FSI send ignore comments #1859 FSI send ignore comments #1859 Nov 11, 2024
Copy link

@vzarytovskii vzarytovskii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any interactions with line numbers and fsi in ionide? I.e. if we strip comments, and then repl will emit a diagnostic, is it navigable back to editor? In this case, it needs to somehow be compensated.

Comment on lines +499 to +503
let private commentRegex =
Regex(@"\/{2,3}.+\S+", RegexOptions.ECMAScript)

let private multilineCommentRegex =
Regex(@"\(\*[\s\S]*?\*\)", RegexOptions.ECMAScript)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should these be compiled?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vzarytovskii these are compiled to JS regexes, and I don't think there's a 'compiled' equivalent on that runtime.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I see

@baronfel
Copy link
Contributor

baronfel commented Nov 12, 2024

@vzarytovskii you make a good point about the compensation - right now the closest thing we have to an 'in-editor' link is the fact that some terminals will look for things that look like a file path/line/column and will auto-link to it. VSCode does do this, so the line numbers might be a bit off.

This is something that would be fixed for real if Ionide managed FSI sessions interactively via the FCS APIs instead of by 'driving' dotnet fsi instances.

@vzarytovskii
Copy link

@vzarytovskii you make a good point about the compensation - right now the closest thing we have to an 'in-editor' link is the fact that some terminals will look for things that look like a file path/line/column and will auto-link to it. VSCode does do this, so the line numbers might be a bit off.

This is something that would be fixed for real if Ionide managed FSI sessions interactively via the FCS APIs instead of by 'driving' dotnet fsi instances.

The "cheapest" solution might be is to not remove comments, but strip them to just //, it will preserve lines, but no need for maintaining offsets.

@@ -494,13 +496,27 @@ module Fsi =
return terminal
}

let private commentRegex =
Regex(@"\/{2,3}.+\S+", RegexOptions.ECMAScript)

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't match the following:

//a
<any number of spaces here>//a<any number of spaces here>

First is quite short, so I suppose it's not an issue, second one might cause some issues?

This will match the following:

let foo = "// asdasdasdasdasdasd"

Which probably is not desired.

Second regex probably has some similar issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants