Skip to content

gh-102033: Fix syntax error in Tools/c-analyzer - #102066

Merged
miss-islington merged 7 commits into
python:mainfrom
sobolevn:issue-102033
Mar 22, 2023
Merged

gh-102033: Fix syntax error in Tools/c-analyzer#102066
miss-islington merged 7 commits into
python:mainfrom
sobolevn:issue-102033

Conversation

@sobolevn

@sobolevnsobolevn commented Feb 20, 2023

Copy link
Copy Markdown
Member

The easiest way to format strings with {} meaningful chars is via %.

Automerge-Triggered-By: GH:ericsnowcurrently

@hauntsaninja

hauntsaninja commented Feb 20, 2023

Copy link
Copy Markdown
Contributor

Hmm, larger question than this PR, but if this has had a syntax error for the last three years, is it still useful?

@sobolevn

sobolevn commented Feb 20, 2023

Copy link
Copy Markdown
MemberAuthor

@hauntsaninja good idea! I went trough all files in c-analyzer and:

  • Removed empty ones
  • Removed ones that are unused (like _dparse, _alt, _state_machine)
  • Removed unused imports
  • Removed unused variables

I double checked that:

  • It does not affect semantics
  • It does not affect runtime

To test that everyting is still working I've triggered 3dc3bd9 with a partial revert of #101657 to test the warning.

However, right now Lib/test/test_check_c_globals.py is disabled :(

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Any idea - why patchcheck fails?

@hauntsaninja

hauntsaninja commented Feb 20, 2023

Copy link
Copy Markdown
Contributor

@sobolevn It wants you to apply this diff:

diff --git a/Tools/c-analyzer/cpython/_capi.py b/Tools/c-analyzer/cpython/_capi.py
index 5584677dae..dc453cb828 100644
--- a/Tools/c-analyzer/cpython/_capi.py+++ b/Tools/c-analyzer/cpython/_capi.py@@ -611,7 +611,7 @@ def _render_item_full(item, groupby, verbose):
yield f' {"filename:":10} {item.relfile}'
for extra in ('kind', 'level'):
#if groupby != extra:
- yield f' {extra+":":10} {getattr(item, extra)}'+ yield f' {extra+":":10} {getattr(item, extra)}'
if verbose:
print(' ---------------------------------------')
for lno, line in enumerate(item.text, item.lno):

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Thank you!

@ericsnowcurrently

Copy link
Copy Markdown
Member

I'm hesitant to do any significant cleanup on this tool before we've re-enabled the CI check for globals. Would you mind if we tabled this until then?

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Sure, seems reasonable!

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Since #102506 was merged, I think we can return to this PR.

Re-running the CI.

@sobolevn

Copy link
Copy Markdown
MemberAuthor

@ericsnowcurrently the CI passed with your new changes. What are the next steps?

@ericsnowcurrentlyericsnowcurrently left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM

This change is almost entirely deleting unused code. I don't expect that any of that code is important to keep at this point. Thanks for doing this!

@sobolevn

Copy link
Copy Markdown
MemberAuthor

Thanks everyone!

Is there anything else I need to do? :)
If not, can someone please click "merge"?

@ericsnowcurrently

Copy link
Copy Markdown
Member

Thanks again, @sobolevn!

Fidget-Spinner pushed a commit to Fidget-Spinner/cpython that referenced this pull request Mar 27, 2023
)
The easiest way to format strings with `{}` meaningful chars is via `%`.
warsaw pushed a commit to warsaw/cpython that referenced this pull request Apr 11, 2023
)
The easiest way to format strings with `{}` meaningful chars is via `%`.
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants

@sobolevn@hauntsaninja@ericsnowcurrently@bedevere-bot@miss-islington