Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3552 +/- ##
==========================================
+ Coverage 97.36% 97.38% +0.01%
==========================================
Files 55 57 +2
Lines 9942 10001 +59
Branches 1823 1836 +13
==========================================
+ Hits 9680 9739 +59
Misses 152 152
Partials 110 110 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
stefan6419846
added
the
needs-change
|
@stefan6419846 ready for code review. The code coverage is picking up comments, I don't know if this is a real code coverage issue or not. |
|
Could you please have a look at the style failures and the coverage first? |
Yes will try and fix the style failures. The coverage seems ok, and is even hightlighting comments. |
Invalid trigger values are not covered correctly. Additionally, rebasing the code on the lastest main might help as the comment indicates that your branch is 42 commits behind, possibly leading to unexpected results. |
Implement the JavaScript action at page-level.
| # Example: Display the page number when the page is closed | ||
| >>> self.add_action("close", JavaScript("app.alert('This is page ' + this.pageNum);"))) | ||
| """ | ||
| if trigger not in {"open", "close"}: |
There was a problem hiding this comment.
Please avoid such large implementations inside the PageObject it possible. I guess we could make the internals of add_action a private method of the Action class and the internals of delete_action a private class method of the `Action class.
There was a problem hiding this comment.
Although it is a large addition, the design as driven by you in an earlier Issue is good. Once all the other action types are added, that adds a lot of code, but this will be outside of PageObject and also in its own test file (test_actions.py).
There was a problem hiding this comment.
The design defined the basic API, not the whole implementation. Thus I still propose to move the actual internal implementation out of the _page.py file and call it accordingly.
There was a problem hiding this comment.
@stefan6419846 I will take your advice that this is the best way for this and implement it. Is there an existing function where we do this, or you can provide a method prototype so I can understand the idea?
Also could you help get the other PR (make any changes you want) done so I can concentrate on this PR?
There was a problem hiding this comment.
Is there an existing function where we do this
See pypdf._writer.PdfWriter.add_attachment.
Also could you help get the other PR (make any changes you want) done so I can concentrate on this PR?
My personal resources are limited, thus I generally try to avoid doing larger changes to PRs not created by me. Exceptions might be critical fixes. I suggest you to mark your PR as draft while you are not working on it to come back to it later, although my usual recommendation would be to wait for one change to be successfully merged before getting started with the next one.
There was a problem hiding this comment.
After addding an Actions interface in this PR, I may add it to other objects. So function overloading will be needed.
For the other one it is ready to merge now and passes the tests. I agree about doing one PR at a time, and will mark this one as draft, as the other one is nearly a year old.
2 participants
Implement the JavaScript action at page-level.