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.

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

Cleanup driveitems.go #8228

Merged
merged 2 commits into from Jan 17, 2024
Merged

Cleanup driveitems.go #8228

merged 2 commits into from Jan 17, 2024

Conversation

kobergj
Copy link
Collaborator

@kobergj kobergj commented Jan 17, 2024

Main intend is to use proto getters to avoid panics.

Fixes #7899

This also fixes some other issues that made my IDE sad and angry.

Signed-off-by: jkoberg <jkoberg@owncloud.com>
@@ -741,7 +740,7 @@ func (g Graph) getCS3UserShareByID(ctx context.Context, permissionID string) (*c
return getShareResp.GetShare(), nil
}

func (g Graph) updateUserShare(ctx context.Context, permissionID string, oldPermission, newPermission *libregraph.Permission) (*libregraph.Permission, error) {
func (g Graph) updateUserShare(ctx context.Context, permissionID string, _, newPermission *libregraph.Permission) (*libregraph.Permission, error) {
Copy link
Contributor

@rhafer rhafer Jan 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please remove the oldPermissions parameter completely from the method signature. AFAICS there is only one caller and this is some leftover that was overlooked during review.

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a little note about the unused parameter. Other than that lgtm.

Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj requested a review from rhafer January 17, 2024 12:22
Copy link

sonarcloud bot commented Jan 17, 2024

Quality Gate Passed Quality Gate passed

The SonarCloud Quality Gate passed, but some issues were introduced.

6 New issues
0 Security Hotspots
72.6% Coverage on New Code
22.2% Duplication on New Code

See analysis details on SonarCloud

Copy link
Contributor

@rhafer rhafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@kobergj kobergj merged commit 903097e into owncloud:master Jan 17, 2024
4 checks passed
@kobergj kobergj deleted the UseProtoGetters branch January 17, 2024 13:40
ownclouders pushed a commit that referenced this pull request Jan 17, 2024
@micbar micbar added this to the Release 5.0.0 milestone Jan 22, 2024
@micbar micbar mentioned this pull request Jan 26, 2024
71 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[graph] driveItems listing results in panic
4 participants