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

[Auth] Migrate setFields behavior to Response initializer #14023

Merged
merged 3 commits into from
Nov 6, 2024

Conversation

ncooke3
Copy link
Member

@ncooke3 ncooke3 commented Nov 4, 2024

#no-changelog

@ncooke3
Copy link
Member Author

ncooke3 commented Nov 4, 2024

@andrewheard, assuming I did this correctly, this is a refactor to address #14012 (review). While it does remove the let, I may have made it more convoluted. WDYT?

@andrewheard
Copy link
Contributor

@andrewheard, assuming I did this correctly, this is a refactor to address #14012 (review). While it does remove the let, I may have made it more convoluted. WDYT?

I think you're right that it was more readable before. If this isn't blocking your plans to make AuthBackend an actor, potentially we could defer to Paul for a second opinion later in the week?

@ncooke3
Copy link
Member Author

ncooke3 commented Nov 5, 2024

@andrewheard, assuming I did this correctly, this is a refactor to address #14012 (review). While it does remove the let, I may have made it more convoluted. WDYT?

I think you're right that it was more readable before. If this isn't blocking your plans to make AuthBackend an actor, potentially we could defer to Paul for a second opinion later in the week?

Yes, SG. You're right that this won't block future improvements. I think we'd have to do a larger refactor to make response handling code better, and I'm unsure if we can make some of the assumptions along the way.

@ncooke3 ncooke3 marked this pull request as draft November 5, 2024 19:32
Copy link
Member

@paulb777 paulb777 left a comment

Choose a reason for hiding this comment

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

Replacing setFields with an init is a nice cleanup and I don't think the call-site is any more convoluted than it already was.

@ncooke3 ncooke3 marked this pull request as ready for review November 6, 2024 19:31
@ncooke3 ncooke3 changed the base branch from main to nc/auth-swift November 6, 2024 22:08
@ncooke3 ncooke3 merged commit 1c82538 into nc/auth-swift Nov 6, 2024
5 checks passed
@ncooke3 ncooke3 deleted the nc/auth-single-2 branch November 6, 2024 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants