-
Notifications
You must be signed in to change notification settings - Fork 1.5k
MAINT: Refactor _page.py #3339
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
base: main
Are you sure you want to change the base?
MAINT: Refactor _page.py #3339
Conversation
993054c
to
9f1cce2
Compare
This is an experiment to see how well Github Copilot works
9f1cce2
to
abb5130
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3339 +/- ##
==========================================
- Coverage 96.73% 96.71% -0.03%
==========================================
Files 54 54
Lines 9073 9096 +23
Branches 1676 1661 -15
==========================================
+ Hits 8777 8797 +20
- Misses 177 180 +3
Partials 119 119 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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 re-use the existing text extraction submodule instead of a new one. Additionally, we should review the coverage.
Just for interest: Did you do a complete review/diff of the changes to see whether anything was changed during refactoring without it being expected?
I did some smoke-tests and all parts I checked were identical. However, I did not check everything. |
I was pleasently surprised how well agent mode of Github copilot worked for this. I essentially gave it #3010 (comment) (rephrased a little bit) and it did almost do all the work. After that I used ruff check / ruff format + fix some mypy issues and that was it :-) |
The missing lines are not new. So I would expect that they are currently missing as well. |
I'll look into it 👍 |
f47f1d9
to
1ed2e38
Compare
@stefan6419846 I feel like this change is too hard to review. What do you think about me doing this in 2 (or more) PRs? In a first PR, I would create |
@MartinThoma The easier to review the code, the better - there surely are similar reasons why some of the larger recent PRs are still in my review queue. |
b85814e
to
f082a56
Compare
f082a56
to
c93aadd
Compare
@MartinThoma What is the current state of this PR? |
This is an experiment to see how well Github Copilot works
The goal of this PR is to make the PageObject easier to understand and maintain.
For the Reviewer