-
-
Notifications
You must be signed in to change notification settings - Fork 4.7k
refactor(IShare): Add typing for IShare #57575
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
Conversation
c63846f to
7ae0c6f
Compare
7ae0c6f to
c9ff2e8
Compare
8b6715c to
cec0a3b
Compare
artonge
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.
For v34?
Yeah feels tiny to risky to do that in 33. Not worth the tiny performance improvements |
cec0a3b to
c1575e9
Compare
|
Converting to draft until we do a branching as this is NC 34 only material |
This might also improve a bit the performance. Signed-off-by: Carl Schwan <carlschwan@kde.org>
c1575e9 to
c8989d8
Compare
| * @since 9.1.0 | ||
| */ | ||
| public function setId($id); | ||
| public function setId(string $id): self; |
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.
Despite the code comment this was int everywhere as you can see in your code and test adjustments…
It breaks Talk share provider according to CI in richdocuments…
So this should receive a proper documentation PR and if possible we allow int for the time being and cast to 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.
Also breaking in deck it seems
Summary
This might also improve a bit the performance as we are spending 2-3% of CPU time in
IShare::setIdTODO
Checklist
3. to review, feature component)stable32)