Skip to content

cpp-linter performing checkup on a new .c file failed #16

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.

Sign up for GitHub

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

Jump to bottom
Closed
shenxianpeng opened this issue Nov 7, 2022 · 13 comments · Fixed by #17
Closed

cpp-linter performing checkup on a new .c file failed #16

shenxianpeng opened this issue Nov 7, 2022 · 13 comments · Fixed by #17
Labels
bug Something isn't working enhancement New feature or request

Comments

@shenxianpeng
Copy link
Contributor

shenxianpeng commented Nov 7, 2022 •
edited
Loading

I am trying to reproduce #18 in this PR shenxianpeng/test-repo#6

But I have another problem.

Performing checkup on p_enemy.c
  INFO:CPP Linter:Running "clang-tidy-14 --export-fixes=clang_tidy_output.yml -checks=boost-* ,bugprone-* ,performance-* ,readability-* ,portability-* ,modernize-* ,clang-analyzer-* ,cppcoreguidelines-* ,-cppcoreguidelines-avoid-magic-numbers ,-readability-magic-numbers --extra-arg= p_enemy.c"
  DEBUG:CPP Linter:Output from clang-tidy:
  
  Traceback (most recent call last):
    File "/home/runner/.local/bin/cpp-linter", line 8, in <module>
      sys.exit(main())
    File "/home/runner/.local/lib/python3.8/site-packages/cpp_linter/run.py", line [101](https://github.com/shenxianpeng/test-repo/actions/runs/3408873516/jobs/5669961676#step:3:106)2, in main
      capture_clang_tools_output(
    File "/home/runner/.local/lib/python3.8/site-packages/cpp_linter/run.py", line 702, in capture_clang_tools_output
      run_clang_format(filename, file, version, style, lines_changed_only)
    File "/home/runner/.local/lib/python3.8/site-packages/cpp_linter/run.py", line 600, in run_clang_format
      for line_range in file_obj["line_filter"][ranges]:
  KeyError: 'line_filter'
  Error: Process completed with exit code 1.

For more details see https://github.com/shenxianpeng/test-repo/actions/runs/3408873516/jobs/5669961676
@shenxianpeng shenxianpeng added the bug Something isn't working label Nov 7, 2022
@shenxianpeng shenxianpeng mentioned this issue Nov 7, 2022
Merged
@2bndy5
Copy link
Contributor

2bndy5 commented Nov 7, 2022 •
edited
Loading

I think there's larger problem at work here. If you set lines-changed-only: true and the event included a diff in for the file, then there absolutely should be a key named line_filter in the dict. The code that creates the key is in filter_out_non_source_files():

cpp-linter/cpp_linter/run.py

Lines 388 to 408 in a23e14d

if "patch" in file.keys():
# get diff details for the file's changes
# ranges is a list of start/end line numbers shown in the diff
ranges: List[List[int]] = []
# additions is a list line numbers in the diff containing additions
additions: List[int] = []
line_numb_in_diff: int = 0
for line in cast(str, file["patch"]).splitlines():
if line.startswith("+"):
additions.append(line_numb_in_diff)
if line.startswith("@@ -"):
hunk = line[line.find(" +") + 2 : line.find(" @@")].split(",")
start_line, hunk_length = [int(x) for x in hunk]
ranges.append([start_line, hunk_length + start_line])
line_numb_in_diff = start_line
elif not line.startswith("-"):
line_numb_in_diff += 1
file["line_filter"] = dict(
diff_chunks=ranges,
lines_added=consolidate_list_to_ranges(additions),
)

All this to say: Why wasn't there a diff included in the event for that file?

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 7, 2022 •
edited
Loading

This wouldn't be problem if I can get the solution for cpp-linter/cpp-linter-action#4 working in the CI runner.

I also thought about switching to requesting the diff format (instead of using the json format) for REST API call that gets the changed files of an event.

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Nov 7, 2022 •
edited
Loading

This might need time to think about and fix by solution for #4. But now we may need to avoid this problem immediately If this temporary solution #17 works.

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 7, 2022

I think the real culprit here is that GitHub didn't bother with a diff because the status of the file was "added". It would be nice if they mentioned this kind of detail in their REST API docs.
https://api.github.com/repos/shenxianpeng/test-repo/pulls/6/files

@shenxianpeng
Copy link
Contributor Author

The failed job is passed this time, but I see there is a notice annotation was added.
See for details https://github.com/shenxianpeng/test-repo/actions/runs/3408873516

Do we need or remove it?

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 8, 2022

That just means that clang-format found problems with the listed lines.

@shenxianpeng
Copy link
Contributor Author

OK, got it. review code I understand, it should also display other messages "Code does not conform to {style_guide} style guidelines.", but it's too long.

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 8, 2022

it should also display other messages "Code does not conform to {style_guide} style guidelines.", but it's too long.

Yes, that's a compromise I made. You cannot make more than 50 annotations in a single workflow run. I think the limit for message content is something larger (can't remember the exact number of bytes). So instead of creating an annotation for every line that didn't pass the checks, I just put the entire list in 1 annotation at the beginning of the file.

@shenxianpeng
Copy link
Contributor Author

Make sense. the largest string length for each annotation seems 4097. Maybe we could only display 4000 lengths of string and others lengths for display "Code does not conform to {style_guide} style guidelines."

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 8, 2022

I thought the title, "Run clang-format on p_enemy.c: p_enemy.c#L1", would have implied that code does not conform to clang-format settings.

@2bndy5
Copy link
Contributor

2bndy5 commented Nov 8, 2022

It might be easier to use an ellipses when the number of lines is too long to fit in the annotation. Maybe something like

File p_enemy.c (lines 15 ... 2423) Code does not conform to {style_guide} style guidelines

@shenxianpeng
Copy link
Contributor Author

shenxianpeng commented Nov 8, 2022 •
edited
Loading

Maybe we could display "Code does not conform to {style_guide} style guidelines." first then with lines, it's easy to change 😃

File p_enemy.c does not conform to {style_guide} style guidelines. (lines 15, 28, 37, 39, 42, 43, 47, 48, 54, 57, 62, 63, 66, 69, 70, 74, 76, 80, 81, 97, 110, 112, 115, 121, 125, 129, 130, 132, 133, 134, 146, 148 ...

@shenxianpeng
Copy link
Contributor Author

Reopen this ticket to for tracking the rest changes

@shenxianpeng shenxianpeng reopened this Nov 8, 2022
@shenxianpeng shenxianpeng mentioned this issue Nov 8, 2022
Merged
@shenxianpeng shenxianpeng added the enhancement New feature or request label Nov 8, 2022
@2bndy5 2bndy5 closed this as completed Nov 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

check dict key before using it cpp-linter/cpp-linter
2 participants
@shenxianpeng @2bndy5