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

Modernize #32

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Modernize #32

wants to merge 2 commits into from

Conversation

tamird
Copy link

@tamird tamird commented Aug 1, 2024

This bumps the version to 0.1.19 because the type of AbortController
is not backwards compatible.

  • Remove obsolete dependencies.
    • abort-controller; AbortController is stable since Node 15.4.0.
    • source-map-support; --enable-source-maps is supported since Node
      12.12.0.
  • Update dependencies.
  • Enable strict mode in tsconfig.json.
  • Use which(..., {nothrow: true}) and distinguish between
    binary-not-in-PATH and binary-access-failed. Log an error.
  • Check for presence of clangd binary in zip archive before extracting.
  • Use native promises APIs instead of promisify.
  • Assign severity to all logs (no more console.log).

The debugging changes here are what motivated the work; I'm having some problems using vscode-clangd on a remote machine where the path to clangd is absolute, and the current implementation doesn't log anything actionable.

@tamird
Copy link
Author

tamird commented Aug 6, 2024

@HighCommander4 would you be willing to review this?

@HighCommander4
Copy link
Contributor

My preference is for changes to a piece of code to be reviewed by the original author of that code, since that person has more familiarity with the code and the reasons it was doing things a certain way.

In this case the original author of most of src/index.ts is @sam-mccall, and accordingly I've added him as a reviewer.

If you don't get a response in a few weeks, please feel free to flag me as a fallback.

We're formatting TypeScript, not C/C++.
This bumps the version to 0.1.19 because the type of `AbortController`
is not backwards compatible.

- Remove obsolete dependencies.
  - abort-controller; `AbortController` is stable since Node 15.4.0.
  - source-map-support; `--enable-source-maps` is supported since Node
    12.12.0.
- Update dependencies.
- Enable strict mode in `tsconfig.json`.
- Use `which(..., {nothrow: true})` and distinguish between
  binary-not-in-PATH and binary-access-failed. Log an error.
- Check for presence of clangd binary in zip archive before extracting.
- Use native promises APIs instead of `promisify`.
- Assign severity to all logs (no more `console.log`).
@tamird
Copy link
Author

tamird commented Aug 6, 2024

Thank you. In the meantime, could you hit the CI once more? I've replaced clang-format with prettier and fixed the formatting (along with a minimal configuration that hews as closely to the current formatting as possible).

@tamird
Copy link
Author

tamird commented Aug 21, 2024

@HighCommander4 could you take a look at this please? It has been 2 weeks now.

@HighCommander4 HighCommander4 self-requested a review August 21, 2024 20:06
@tamird
Copy link
Author

tamird commented Sep 9, 2024

@HighCommander4 3 more weeks elapsed. Could you please give this a review?

@HighCommander4
Copy link
Contributor

@tamird I'm sorry, I'm a volunteer contributor and I have limited free time to contribute to clangd.

I get a higher volume of review requests than I'm able to keep up with. I will try to get to this, but I can't promise any particular time frame.

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.

2 participants