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

Servantify Galley Teams [2/2] #2277

Merged
merged 8 commits into from
Apr 13, 2022
Merged

Conversation

stephen-smith
Copy link
Contributor

JIRA:

Checklist

  • The PR Title explains the impact of the change.
  • The PR description provides context as to why the change should occur and what the code contributes to that effect. This could also be a link to a JIRA ticket or a Github issue, if there is one.
  • changelog.d contains the following bits of information (details):
    • A file with the changelog entry in one or more suitable sub-sections. The sub-sections are marked by directories inside changelog.d.

@stephen-smith stephen-smith self-assigned this Apr 11, 2022
@stephen-smith stephen-smith temporarily deployed to cachix April 11, 2022 19:46 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 12, 2022 08:14 Inactive
Copy link
Contributor

@pcapriotti pcapriotti left a comment

Choose a reason for hiding this comment

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

Looks good!

One thing I'd like to improve is the use of NoContent, which is a bit of a hack in servant, and it contaminates the handler code with servant-specific nonsense. We have created a MultiVerb combinator to solve this and other limitations of servant, and here it works pretty well (in its MultiVerb1 incarnation, which is for a single response).

I've taken the liberty of adding a commit to this PR which removes the uses of NoContent. As you can see, a lot of meaningless glue code around the handler just goes away.

@fisx
Copy link
Contributor

fisx commented Apr 12, 2022

Looks good!

One thing I'd like to improve is the use of NoContent, which is a bit of a hack in servant, and it contaminates the handler code with servant-specific nonsense. We have created a MultiVerb combinator to solve this and other limitations of servant, and here it works pretty well (in its MultiVerb1 incarnation, which is for a single response).

I've taken the liberty of adding a commit to this PR which removes the uses of NoContent. As you can see, a lot of meaningless glue code around the handler just goes away.

Definitely a good change, but the glue code you removed was mostly historical: when we were preparing the wai-route code for servant in the past, we had two functions: a wrapper handlerH that had a signature accomodating wai-route, and handler with a signature accomodating servant and containing all the application logic. Or the other way around, not sure at the moment, there has been so much refactoring. :)

With the removal of wai-route, we probably want to remove all the wrappers.

@pcapriotti pcapriotti temporarily deployed to cachix April 12, 2022 10:37 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 12, 2022 12:34 Inactive
@stephen-smith
Copy link
Contributor Author

With the removal of wai-route, we probably want to remove all the wrappers.

Some I've been able to drop and call the underlying function directly. Some have status code manipulation or Maybe handling, and I've been leaving that in the ...H function. I haven't looked at the NoContent elimination, but that should let me remove a few more ...H functions that were just converting () to NoContent.

@stephen-smith stephen-smith force-pushed the boyd.smith/servantify-galley-teams-2 branch from 118809c to 32313ce Compare April 12, 2022 14:54
@stephen-smith stephen-smith temporarily deployed to cachix April 12, 2022 14:54 Inactive
@pcapriotti pcapriotti temporarily deployed to cachix April 13, 2022 06:00 Inactive
@pcapriotti pcapriotti merged commit 281bcba into develop Apr 13, 2022
@pcapriotti pcapriotti deleted the boyd.smith/servantify-galley-teams-2 branch April 13, 2022 08:45
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.

None yet

3 participants