Skip to content

Add new functionality #275

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

Open
wants to merge 5 commits into
base: dev
Choose a base branch
from
Open

Conversation

anushkavidanage
Copy link
Collaborator

@anushkavidanage anushkavidanage commented May 27, 2025

Pull Request Details

What issue does this PR address

Checklist

Complete the check-list below to ensure your branch is ready for PR.

Flutter Style Guide: https://survivor.togaware.com/gnulinux/flutter-style.html

  • Screenshots included in linked issue
  • Changes adhere to the team style and coding guideline
  • No confidential information
  • No duplicated content
  • No lint check errors related to your changes (make prep or flutter analyze lib)
  • Pre-exisiting lint errors noted: [HERE]
  • Tested on at least one device
    • Android Phone
    • Android Emulator
    • Chrome on Android
    • Chrome
    • iOS
    • Linux
    • MacOS
    • Windows
  • Added 2 reviewers (or 1 for private repositories then they add another)

Finalising

Once PR discussion is complete and 2 reviewers have approved:

  • Merge dev into the branch
  • Resolve any conflicts
  • Add one line summary into CHANGELOG.md
  • Bump appropriate version number in pubspec.yaml
  • Push to git repository and review
  • Merge PR into dev

Copy link
Collaborator

@cdawei cdawei left a comment

Choose a reason for hiding this comment

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

@@ -51,6 +51,7 @@ import 'package:solidpod/src/solid/utils/misc.dart';
/// representing the result of the operation.
Future<String> setPermissionAcl(
String resourceUrl,
String ownerWebId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

For methods that now take ownerWebID and removerWebId as required positional parameters:

  1. Will callers that still use the old signatures fail to compile?
  2. Could we potentially make these new parameters optional named arguments (with sensible defaults) to preserve source compatibility?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks Ashley, this, unfortunately, cannot be made optional. The old function did not have the functionality to set permission when the permission giver and the owner are two different people. So adding that functionality means we have to stop using the old version.

@cdawei cdawei requested a review from gjwgit June 11, 2025 00:49
@cdawei cdawei mentioned this pull request Jun 20, 2025
22 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.

3 participants