Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Cleanup comments code #35424

Merged
merged 3 commits into from
Jun 20, 2023
Merged

Cleanup comments code #35424

merged 3 commits into from
Jun 20, 2023

Conversation

CarlSchwan
Copy link
Member

  • Fix various psalm issues
  • Add as much typing as possible while preserving stable API

@CarlSchwan CarlSchwan added the 3. to review Waiting for reviews label Nov 25, 2022
@CarlSchwan CarlSchwan added this to the Nextcloud 26 milestone Nov 25, 2022
@CarlSchwan CarlSchwan requested review from vitormattos and a team November 25, 2022 10:21
@CarlSchwan CarlSchwan self-assigned this Nov 25, 2022
@CarlSchwan CarlSchwan requested review from ArtificialOwl, icewind1991 and blizzz and removed request for a team November 25, 2022 10:21
github-advanced-security[bot]
github-advanced-security bot found potential problems Nov 25, 2022
View reviewed changes
apps/comments/lib/Listener/CommentsEntityEventListener.php
public function handle(Event $event): void {
if (!($event instanceof CommentsEntityEvent)) {
// Unrelated
return;
}

$event->addEntityCollection('files', function ($name) {
$nodes = \OC::$server->getUserFolder()->getById((int)$name);
$event->addEntityCollection('files', function ($name): bool {

Check notice

Code scanning / Psalm

MissingClosureParamType

Parameter $name has no provided type
apps/comments/lib/Listener/CommentsEntityEventListener.php
$event->addEntityCollection('files', function ($name) {
$nodes = \OC::$server->getUserFolder()->getById((int)$name);
$event->addEntityCollection('files', function ($name): bool {
$nodes = $this->rootFolder->getUserFolder($this->userId)->getById((int)$name);

Check notice

Code scanning / Psalm

PossiblyNullArgument

Argument 1 of OCP\Files\IRootFolder::getUserFolder cannot be null, possibly null value provided
apps/dav/lib/Comments/EntityCollection.php Fixed Show fixed Hide fixed
blizzz
blizzz approved these changes Nov 25, 2022
View reviewed changes
apps/comments/lib/Notification/Listener.php Show resolved Hide resolved
lib/public/Comments/ICommentsManager.php Outdated Show resolved Hide resolved
@CarlSchwan CarlSchwan requested a review from come-nc December 1, 2022 11:30
@szaimen
Copy link
Contributor

szaimen commented Dec 3, 2022 •
edited
Loading

image

@szaimen szaimen added 2. developing Work in progress and removed 3. to review Waiting for reviews labels Dec 3, 2022
@come-nc
Copy link
Contributor

come-nc commented Dec 5, 2022

image

Looks like a timestamp being off by 1 second. I cannot reproduce the failure locally.

@szaimen
Copy link
Contributor

szaimen commented Dec 5, 2022

/rebase

@blizzz blizzz mentioned this pull request Feb 1, 2023
Merged
@skjnldsv skjnldsv mentioned this pull request Feb 23, 2023
Merged
@blizzz blizzz mentioned this pull request Mar 7, 2023
Merged
@blizzz blizzz modified the milestones: Nextcloud 26, Nextcloud 27 Mar 9, 2023
@szaimen
Copy link
Contributor

szaimen commented Apr 17, 2023

conflicts :/

@skjnldsv skjnldsv mentioned this pull request May 3, 2023
Merged
@come-nc come-nc assigned come-nc and unassigned CarlSchwan May 4, 2023
@come-nc come-nc removed the 2. developing Work in progress label May 4, 2023
@come-nc come-nc added the 4. to release Ready to be released and/or waiting for tests to finish label May 4, 2023
@come-nc come-nc modified the milestones: Nextcloud 27, Nextcloud 28 May 4, 2023
@szaimen szaimen added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels May 16, 2023
@szaimen
Copy link
Contributor

szaimen commented May 16, 2023

Tests failing

@come-nc come-nc force-pushed the cleanup/comments branch from e2e9717 to 3a7e5f1 Compare May 22, 2023 12:44
@szaimen szaimen added 4. to release Ready to be released and/or waiting for tests to finish and removed 2. developing Work in progress labels May 22, 2023
github-advanced-security[bot]
github-advanced-security bot found potential problems May 22, 2023
View reviewed changes
apps/comments/lib/Notification/Listener.php
*
* @param array $mentions
* @return string[] containing the mentions, e.g. ['alice', 'bob']
* @return list<string> containing the mentions, e.g. ['alice', 'bob']

Check notice

Code scanning / Psalm

MoreSpecificReturnType

The declared return type 'list<string>' for OCA\Comments\Notification\Listener::extractMentions is more specific than the inferred return type 'list<mixed>'
@szaimen
Copy link
Contributor

szaimen commented May 23, 2023

CI failure...

@szaimen
Copy link
Contributor

szaimen commented May 25, 2023

/rebase

@szaimen
Copy link
Contributor

szaimen commented May 26, 2023

CI failure related afaics

@szaimen szaimen added 2. developing Work in progress and removed 4. to release Ready to be released and/or waiting for tests to finish labels May 26, 2023
@artonge
Copy link
Contributor

artonge commented Jun 1, 2023

/rebase

CarlSchwan and others added 3 commits June 20, 2023 10:55
@CarlSchwan @come-nc
Cleanup comments code
9725d00 
- Fix various psalm issues
- Add as much typing as possible while preserving stable API

Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@CarlSchwan @come-nc
Improve typing in ICommentsManager
77355a8 
Signed-off-by: Carl Schwan <carl@carlschwan.eu>
@come-nc
Throw if creation date is read before inserting into database
7aa97dc 
Signed-off-by: Côme Chilliet <come.chilliet@nextcloud.com>
@come-nc come-nc force-pushed the cleanup/comments branch from 7fa58da to 7aa97dc Compare June 20, 2023 10:15
@come-nc come-nc merged commit d9e9d4e into master Jun 20, 2023
@come-nc come-nc deleted the cleanup/comments branch June 20, 2023 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-nc come-nc come-nc approved these changes

@blizzz blizzz blizzz approved these changes

@nickvergessen nickvergessen nickvergessen left review comments

@icewind1991 icewind1991 icewind1991 approved these changes

@vitormattos vitormattos Awaiting requested review from vitormattos

@ArtificialOwl ArtificialOwl Awaiting requested review from ArtificialOwl

Assignees

@come-nc come-nc

Labels
2. developing Work in progress technical debt
Projects
None yet
Milestone
Nextcloud 28
Development

Successfully merging this pull request may close these issues.
7 participants
@CarlSchwan @szaimen @come-nc @artonge @nickvergessen @icewind1991 @blizzz