Improve the names of a few commonly used APIs #2014
Conversation
| /// - Note: You can only call this function once, repeated calls will result in a | ||
| /// ``RuntimeError`` being thrown. | ||
| public func run() async throws { | ||
| public func serve() async throws { |
There was a problem hiding this comment.
I am a bit wary of renaming this from run to serve. While I understand what you are getting at. We do have many libraries that now adopted the run method name to align with what Lifecycle expects. If you rename this here and conform to Service in the future your type will have two methods that do the same.
There was a problem hiding this comment.
FWIW I agree with Franz here, I think it is more important to be consistent with the rest of the ecosystem on this.
There was a problem hiding this comment.
run is less clear and is worse for users which don't depend on lifecycle. Users would have to provide shims to use Lifecycle anyway (to avoid retroactive conformance). If we provide conformance in the future (e.g. via package traits) then having a run() method calling through to serve() isn't an issue.
There was a problem hiding this comment.
From the perspective of a user using Lifecycle the names of these methods don't matter at all because they won't be called directly by the user.
From the perspective of a user not using Lifecycle, these methods matter a lot: they will be called by the user so they should be clear.
There was a problem hiding this comment.
I still remain unconvinced. serve and run are pretty close in my mind. Even leaving Lifecycle aside just the proliferation of run methods across the ecosystem is IMO enough to justify aligning. It becomes very natural that you call run on your server/clients when you don't use something like Lifecycle.
I don't want to block this PR on this and you can merge over my unconvincedness.
There was a problem hiding this comment.
Noted. I'm pretty strongly against aligning for the sake of aligning on a generic name: I think we should pick names that express their intent and provide the most clarity, the APIs should be approachable for all users including those new to Swift and SoS!
| /// | ||
| /// Calling this on a server which is already stopping or has stopped has no effect. | ||
| public func stopListening() { | ||
| public func beginGracefulShutdown() { |
There was a problem hiding this comment.
I would suggest using triggerGracefulShutdown to align with https://github.com/swift-server/swift-service-lifecycle/blob/24c800fb494fbee6e42bc156dc94232dc08971af/Sources/ServiceLifecycle/ServiceGroup.swift#L167.
Motivation: Naming is important; it should be clear and concise. Modifications: The follow renames all offer more precise names: - Rename `server.run()` to `server.serve()` - Rename `server.stopListening()` to `server.beginGracefulShutdown()` - Rename `client.close()` to `client.beginGracefulShutdown()` Result: Clearer APIs
glbrntt
force-pushed
the
naming-improvements
branch
from
267f7ba to
c6336bb
Compare
3 participants
Motivation:
Naming is important; it should be clear and concise.
Modifications:
The follow renames all offer more precise names:
server.run()toserver.serve()server.stopListening()toserver.beginGracefulShutdown()client.close()toclient.beginGracefulShutdown()Result:
Clearer APIs