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

Enhance LSP for multi-editor support #38

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

rahulyadav-57
Copy link
Member

  • Support for Editor/IDE with file system access
  • Support for Monaco and other browser-based editors

To use the LSP with other editors, refer to tact-extracted-ls.

Resolves #35

- Moved code formatting from the VSCode extension to the LSP server.
- Removed VSCode specific dependencies from code formatting to create a unified, editor-agnostic approach.
- Updated runCompilation method to correctly handle files opened without a workspace directory, ensuring seamless functionality in single-file contexts.
- Switched document sync strategy from full to incremental for improved performance.
@logvik
Copy link
Member

logvik commented Jun 17, 2024

Thank you for your contribution! I'll check it next weekend.

@novusnota
Copy link
Member

This PR also resolves #29, as it enables formatting capabilities in the LS 🚀

Copy link
Member

@logvik logvik left a comment

Choose a reason for hiding this comment

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

Sorry for the delay, looks good, just we need to resolve few moments...

@@ -68,12 +69,12 @@ async function validate(document: TextDocument) {
let newDocumentsWithErrors: any = [];
for (let fileName in compileErrorDiagnostics) {
newDocumentsWithErrors.push(URI.file(fileName).path);
connection.sendDiagnostics({diagnostics: compileErrorDiagnostics[fileName], uri: URI.file(fileName).path});
connection.sendDiagnostics({diagnostics: compileErrorDiagnostics[fileName], uri: "file://"+URI.file(fileName).path});
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure about this. Will it work for web version? What for should we need add "file://"?

Copy link
Member Author

Choose a reason for hiding this comment

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

For the web version, similar to the Monaco editor, a few additional changes are required. I'll push those by this weekend.

}
let difference = documentsWithErrors.filter((x: any) => !newDocumentsWithErrors.includes(x));
// if an error is resolved, we must to send empty diagnostics for the URI contained it;
for(let item in difference) {
connection.sendDiagnostics({diagnostics: [], uri: difference[item]});
connection.sendDiagnostics({diagnostics: [], uri: "file://"+difference[item]});
Copy link
Member

Choose a reason for hiding this comment

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

Same as "file://" above.

@@ -39,14 +39,23 @@ export class TactCompiler {
}

const pathKey = path.relative( this.rootPath, args.file).replaceAll('\\','/');

let rootPath = this.rootPath;
Copy link
Member

Choose a reason for hiding this comment

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

Could we change let rootPath = this.rootPath; on let localRootPath = this.rootPath; ? just because we can misunderstand later this.rootPath and rootPath
The same below by code. We can leave this, of course, just to better read the code.
Please decide.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes that would be a better.

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.

Refactor LSP for compatibility with multiple code editors or IDE
3 participants