Skip to content

Comments

Expose members of NKRequestOptions#142

Merged
i2h3 merged 1 commit intomainfrom
feature/exposed-nkrequestoptions
May 26, 2025
Merged

Expose members of NKRequestOptions#142
i2h3 merged 1 commit intomainfrom
feature/exposed-nkrequestoptions

Conversation

@claucambra
Copy link
Contributor

Needed in NextcloudFileProviderKit to mock pagination behaviour of real server

@claucambra claucambra self-assigned this May 19, 2025
@claucambra claucambra force-pushed the feature/exposed-nkrequestoptions branch 3 times, most recently from 1f82bc3 to 91623aa Compare May 19, 2025 07:02
@claucambra claucambra changed the title Expose members of, and structify, NKRequestOptions Expose members of NKRequestOptions May 19, 2025
Copy link
Collaborator

@i2h3 i2h3 left a comment

Choose a reason for hiding this comment

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

It looks fine to me, though I was thinking for a moment why in example contentType is not a constant either (breaking change). It is part of the initializer and has a default value of nil. I did not check call points but I assume those affected can just be extended with the initializer argument instead of mutating the property afterwards as I saw it. Then semantics and performance would benefit, as far as I know.

Out of scope for this pull request: At this point I ask myself why use reference semantics here at all because this would be fine as a plain struct. 🤔

…ible)

Signed-off-by: Claudio Cambra <claudio.cambra@nextcloud.com>
@i2h3 i2h3 force-pushed the feature/exposed-nkrequestoptions branch from 91623aa to 6c938be Compare May 26, 2025 07:25
@i2h3 i2h3 enabled auto-merge (rebase) May 26, 2025 07:26
@i2h3 i2h3 merged commit 8a46a1a into main May 26, 2025
4 checks passed
@i2h3 i2h3 deleted the feature/exposed-nkrequestoptions branch May 26, 2025 07:31
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.

2 participants