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

Use Uri instead of FileName for several fsharp/ requests #949

Merged
merged 5 commits into from May 29, 2022

Conversation

Booksbaum
Copy link
Contributor

@Booksbaum Booksbaum commented May 29, 2022

In Ionide quite a lot of commands don't work in untitled documents.
Reason: FileName instead of Uri is used.

Additional to changes here in FSAC, clients must make some adjustments too.
For Ionide these are in ionide/ionide-vscode-fsharp#1717

Changes:

  • fsharp/FileParsed: returns URI instead of FileName. No type change (is PlainNotification), but content is now URI
  • FSharpPipelineHintRequest, HighlightingRequest, FSharpLiterateRequest use now TextDocumentIdentifier:
    • Prev: { FileName: string }
    • Now: { TextDocument: TextDocumentIdentifier }

Note: This results in changed API
-> as mentioned above: Clients must be adjusted


Additional Fix:

  • Off-by-One column in fsharp/f1Help & fsharp/documentation (used in Info Panel)

In Ionide: used for Info Panel
Change `HighlightingRequest`, `FSharpLiterateRequest` to use Uri too
* But neither of these two is actually used

Note: This commit results in changed API:
* Prev: `{ FileName: string }`
* Now: `{ TextDocument: TextDocumentIdentifier }`
-> Clients must be adjusted
@baronfel
Copy link
Contributor

Ideally I'd like to see tests over areas that change (just as part of a process of greening up the codebase overall over time), but I've tested these manually and the scope of the changes overall are minor.

Personally I'd also like to remove the entire PlainNotification type over time, which requires the kind of cross-repo synchronization that you had to do for this effort, but that's a later effort. Maybe after #945 :)

Thank you again for your attention to detail here!

@baronfel baronfel merged commit 754dd2d into ionide:main May 29, 2022
@Booksbaum Booksbaum deleted the ionide-untitled-uri branch May 30, 2022 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants