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

make Server::start infallible and add fn builder() #1137

Merged
merged 4 commits into from May 28, 2023

Conversation

niklasad1
Copy link
Member

@niklasad1 niklasad1 commented May 26, 2023

Server::start was infallible to let's change the API to reflect that.

@niklasad1 niklasad1 requested a review from a team as a code owner May 26, 2023 14:04
@niklasad1 niklasad1 added the breaking change Breaking change in the public APIs label May 26, 2023
Copy link
Collaborator

@jsdw jsdw left a comment

Choose a reason for hiding this comment

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

LGTM! The only reason to keep the result would be if we thought it might become fallible again, but best not to anticipate such things anyway :)

@niklasad1
Copy link
Member Author

niklasad1 commented May 26, 2023

Yeah, I'm quite tempted of removing the entire start API then just start the server from the Builder but it may useful to manage when the server is started, not sure.

@jsdw
Copy link
Collaborator

jsdw commented May 26, 2023

Having a guide gander, it looks like you may as well keep a separate thing to start it offhand; I tend to feel that breaking functionality apart like this is a good thing (and then providing higher level stuff to do the common cases where applicable)

(I'd probably add a Server::builder() method though as a random thought because that might be a little cleaner than importing a separate Builder type to use to build it, and if I'm looking to make a thing I usually don't think to look for a separate type to construct it)

@niklasad1 niklasad1 changed the title refactor(server): make Server::start infallible refactor(server): make Server::start infallible and add fn builder() May 28, 2023
@niklasad1 niklasad1 merged commit e81bdae into master May 28, 2023
15 checks passed
@niklasad1 niklasad1 deleted the na-server-api-remove-needless-result branch May 28, 2023 10:32
@niklasad1
Copy link
Member Author

I will remove the start API in another PR.

It's quite messy to migrate all tests.

@niklasad1 niklasad1 changed the title refactor(server): make Server::start infallible and add fn builder() make Server::start infallible and add fn builder() May 28, 2023
@lexnv lexnv mentioned this pull request Jul 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Breaking change in the public APIs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants