Skip to content

refactor: make handle resolution methods return non-optional values - #5

Merged
mattmassicotte merged 3 commits into
ATProtoKit:mainfrom
nnabeyang:fix/non-optional-handle-resolution
May 28, 2026
Merged

refactor: make handle resolution methods return non-optional values#5
mattmassicotte merged 3 commits into
ATProtoKit:mainfrom
nnabeyang:fix/non-optional-handle-resolution

Conversation

@nnabeyang

Copy link
Copy Markdown
Contributor

This PR refactors the handle resolution methods in ATResolver to return non-optional values.

Previously, didForHandle(_:) and resolveHandle(_:) returned Optional values, requiring callers to handle nil cases. This change updates these methods to return non-optional String and ResolvedData types, delegating failure handling to Swift’s error handling mechanism (throws).

@mattmassicotte

Copy link
Copy Markdown
Contributor

Thanks for taking a look at this. I'm always torn on stuff like this. Does "no did" mean throw or mean "nil"? I'm tempted to say that an Optional result does in fact make sense here. What do you think about that?

@nnabeyang

Copy link
Copy Markdown
Contributor Author

@mattmassicotte Thanks for the feedback! I totally understand being torn on "throw vs nil" for this kind of domain logic.

The main motivation behind this PR was that, in the current implementation, didForHandle never actually returns nil—it either succeeds with a String or throws an error from underlying methods like didForDomain. The current String? type forces the caller to handle a nil case that never actually happens in the throwing path.

@mattmassicotte

Copy link
Copy Markdown
Contributor

Sorry for taking so long here. You're right, the current implementation doesn't ever return nil. And I think that's actually a bug! It does not make sense that these methods should be forced to throw if a nonexistent handle is used as input.

I think the API is actually correct, and the implementation is the problem!

However, that's an issue for this PR. Do you have any interest in persisting here? I completely understand if you don't and I don't want to waste any more of your time on it.

@nnabeyang

nnabeyang commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@mattmassicotte You're exactly right. Treating a nonexistent handle as a domain-level nil (rather than a system-level throw) makes perfect sense. I've updated the PR to implement this behavior.
a00cc7f
7190f28

@nnabeyang
nnabeyang force-pushed the fix/non-optional-handle-resolution branch from aed7c00 to 7190f28 Compare May 26, 2026 23:09
@mattmassicotte

Copy link
Copy Markdown
Contributor

This makes a lot more sense to me. Thanks so much for taking it on, and being patient while we figured out what the right solution was.

@mattmassicotte

Copy link
Copy Markdown
Contributor

(The CI failure is unrelated. I think at least one successful run is sufficient to validate that the change is reasonable.)

@mattmassicotte
mattmassicotte merged commit bfcc777 into ATProtoKit:main May 28, 2026
2 of 7 checks passed
@nnabeyang
nnabeyang deleted the fix/non-optional-handle-resolution branch May 28, 2026 21:50
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