Skip to content

Conversation

lljbash
Copy link
Owner

@lljbash lljbash commented Jan 7, 2025

Changelog

Features

Refactor

  • Rewrote clangd integration with an async LSP client, removing pylspclient.

Misc

  • Restructured argument parsing.
  • Removed hard dependency on tqdm.

@lljbash lljbash changed the title V1.1.0 feat: v1.1.0 Jan 7, 2025
@lljbash lljbash mentioned this pull request Jan 7, 2025
jmpfar and others added 5 commits January 26, 2025 16:19
See
https://iterm2.com/feature-reporting/Hyperlinks_in_Terminal_Emulators.html
for information about terminal hyperlinks. This is supported by multiple
terminals and makes it easier to find out more information about a
failing check.

This is the equivalent of PR #9 rebased on branch 1.1 (closes #9)

---------

Co-authored-by: Lingjie Li <[email protected]>
Resolve #14

---------

Co-authored-by: Lingjie Li <[email protected]>
…12)

* Adds a --line-filter argument support similar to clang tidy. This
allows filtering of diagnostics on per file and per line range basis
* Add clangd-tidy-diff a CLI command similar to clang-tidy-diff, which
runs clangd-tidy on all files changed in a unified diff and filters the
diagnostics to only contain those from changed lines. This allows easier
onboarding for large existing codebases.

Inspired by clang-tidy-diff script from the llvm project
(https://clang.llvm.org/extra/doxygen/clang-tidy-diff_8py_source.html).
This allows adding a CI check of only the changes lines

This PR is the equivalent of PR #8 rebased on branch 1.1

closes #8

---------

Co-authored-by: Lingjie Li <[email protected]>
Comment on lines 29 to 33
def allows(self, start: int, end: int) -> bool:
return any(
self._interval_intersect(line_range, LineRange(start, end))
for line_range in self.line_ranges
)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you simplified my code really nicely in a few places such as the introduction of the LineRange class, but my original intention in this function was to be as backwards compatible with the original clang-tidy format

see here https://github.com/llvm/llvm-project/blob/980d66caae62de9b56422a2fdce3f535c2ab325f/clang-tools-extra/clang-tidy/ClangTidyDiagnosticConsumer.cpp#L463-L479

It is my understanding the logic is a bit more complex, such as it supports partial file paths and mentioning files without a specific file range

Copy link
Owner Author

Choose a reason for hiding this comment

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

Oh, I didn't realize this was an original clang-tidy feature. I'll work on aligning them.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hey @jmpfar I have fixed the compatiblity issue. Would you be willing to help with a final test?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sure, tell me what you want me to test

One thing that might be an issue:

# EXTRA: don't touch non-clang-tidy diagnostics
if diagnostic.source is not None and diagnostic.source != "clang-tidy":
return True
# EXTRA: filter out clang-tidy diagnostics without source and code
if diagnostic.source is None and diagnostic.code is None:
return False

I guess you had to do that because of the new clang format diagnostics (?) but this causes issues with the unused-includes/missing-includes diagnostics.
I'd like to have these filtered out as well, because the current situation makes these show up even when they're not part of the diff.

In my very quick tests, I also seen this behavior happen with the ovl_no_viable_oper diagnostic, which appears to be a non-clang-tidy analytic, though I am not sure where it is coming from (clangd?)

Copy link
Owner Author

@lljbash lljbash Apr 2, 2025

Choose a reason for hiding this comment

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

though I am not sure where it is coming from (clangd?)

Adding -c flag will show the source.

unused-includes/missing-includes diagnostics.

They come from clangd source.

ovl_no_viable_oper diagnostic

I didn't filter out clang diagnostics to align with clang-tidy, but after reconsidering, I now think it's better to keep only clang-format diagnostics unfiltered.

Please check my latest update.

Copy link
Owner Author

@lljbash lljbash Apr 2, 2025

Choose a reason for hiding this comment

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

Sure, tell me what you want me to test

Thank you! Just check if it works in your cases.

For example, I recently discovered a bug with the exit code caused by invisible diagnostics (real error filtered out by line filter, and support info filtered out by formatter).

Copy link
Contributor

Choose a reason for hiding this comment

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

I did a few more simple tests with my workflow and it looks like it's working well, thanks for your work!

@lljbash lljbash force-pushed the v1.1.x branch 2 times, most recently from b76ef5f to 5e8ce45 Compare April 1, 2025 10:34
@lljbash lljbash requested a review from jmpfar April 3, 2025 05:43
@lljbash lljbash merged commit 1cfb383 into master Apr 7, 2025
3 checks passed
@lljbash lljbash deleted the v1.1.x branch April 7, 2025 07:00
@lljbash
Copy link
Owner Author

lljbash commented Apr 7, 2025

closes #17

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