-
Notifications
You must be signed in to change notification settings - Fork 1
Add script for crate structure comparison #63
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
Add script for crate structure comparison #63
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces a new Python script for analyzing and comparing Rust crate structure, specifically focusing on coverage checking. The script can parse crate structure data, filter by visibility and item types, and compare results with reference data to track coverage progress.
Key changes:
- Added comprehensive crate structure analysis functionality with tree-based data representation
- Implemented filtering capabilities for visibility levels and item types (functions, structs, enums, etc.)
- Added reference comparison feature to track coverage changes over time
Comments suppressed due to low confidence (1)
scripts/coverage_checker.py:345
- Variable name 'all' shadows the built-in function. Consider using 'total' or 'total_nodes' instead.
all = tree.count_leaf_nodes()
PiotrKorkus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Take a look into copilot comments. They aren't that bad and even caught a bug.
I would suggest following structure:
- create an extra directory
coverage_checkerin scripts and place script there - add bash script with command to generate input file
We will need some docstrings for sure.
Let's have a call, as when I try to run it I get quite messy tabs in the output.
scripts/coverage_checker.py
Outdated
| with open(args.input, "r") as f: | ||
| input_lines = f.readlines() | ||
| else: | ||
| input_lines = sys.stdin.readlines() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Lets stop using stdin. Arguments are all we need
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
scripts/coverage_checker.py
Outdated
|
|
||
| self.data = line_parts[0] | ||
| self.comment = line_parts[-1] if len(line_parts) > 1 else None | ||
| self.tab_count = line.count("\t") if self.comment is not None else None |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if self.comment else None is enough
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
scripts/coverage_checker.py
Outdated
| if token.startswith("pub"): | ||
| return Visibility(token) | ||
|
|
||
| raise RuntimeError("Visibility not detected for: " + data_line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
make it f-string
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
scripts/coverage_checker.py
Outdated
| if found != -1: | ||
| return Entry(line, item_type, found) | ||
|
|
||
| raise RuntimeError("No known item type found for parsed line: " + line) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
f-string here as well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
arkjedrz
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I don't think parsing languages using scripts is a good idea. In my experience - it is working shabby at best, and causing unsustainable burden to maintain at worst.
- I feel there might be an overlap with what is expected with https://github.com/cargo-public-api/cargo-public-api
- I'm missing any kind of rationale or example what's this script for.
- I was unable to find one file from Persistency repo that will work with this scripts.
| @@ -0,0 +1,346 @@ | |||
| import argparse | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing shebang and docstring.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- File is missing exe rights.
- Please take
ruffrules from impl: add pre-commit hooks #62 and runruffon this file. I get some issues reported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still to check
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added exe rights
- Fixed or suppressed issues found by ruff
scripts/coverage_checker.py
Outdated
|
|
||
|
|
||
| class TreeNode: | ||
| def __init__(self, entries: list[Entry], ndx: Indexer, parent: Union["TreeNode", None] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Union["TreeNode", None] -> "TreeNode" | None
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have tried that initially, but looks that incomplete type is not supported in this syntax:
File "/home/igor/repos/testing_tools/scripts/coverage_checker.py", line 132, in TreeNode
def __init__(self, entries: list[Entry], ndx: Indexer, parent: "TreeNode" | None = None):
~~~~~~~~~~~^~~~~~
TypeError: unsupported operand type(s) for |: 'str' and 'NoneType'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it won't work since we are referring to the type through string literal. You can leave it as it is, or use Optional["TreeNode"] but | wont work
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Used Optional
f81630a to
95da068
Compare
|
95da068 to
d795ea1
Compare
PiotrKorkus
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add information about setting tabs 4 for best output representation
scripts/coverage_checker/README.md
Outdated
| `cd <inc_orchestrator_repo>` | ||
| `cargo modules structure --package orchestration | sed 's/\x1b\[[0-9;]*m//g' > input_report.txt` - sed is required to remove colored output | ||
| `python3 scripts/coverage_checker.py -r <older_orchestration_report> input_report.txt` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- It doesn't look good when rendered, replace it with single block of code ```.
- Path in commands shall be relative to README, so just
coverage_checker.py - Also please install https://marketplace.visualstudio.com/items?itemName=DavidAnson.vscode-markdownlint and solve issues
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Added double spaces at the end to force new lines. Do not switched to block to avoid having there also explanation for the command.
- Fixed
- Done
scripts/coverage_checker.py
Outdated
|
|
||
|
|
||
| class TreeNode: | ||
| def __init__(self, entries: list[Entry], ndx: Indexer, parent: Union["TreeNode", None] = None): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep, it won't work since we are referring to the type through string literal. You can leave it as it is, or use Optional["TreeNode"] but | wont work
|
d795ea1 to
e76683e
Compare
Added in the readme |
e76683e to
fe727ed
Compare
Add script for crate structure analysis and comparison with older results