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

Go back to non-parameterized resource classes for DW servers #232

Merged
merged 1 commit into from Apr 10, 2019

Conversation

Projects
None yet
2 participants
@kelnos
Copy link
Member

commented Apr 10, 2019

Arguably the old way made the generated code a little nicer to read, but the aesthetics for the user were ugly with the type param, and especially the wildcard type param in the handler signatures.

This effectively reverts acd9f59

Contributing to Twilio

All third party contributors acknowledge that any contributions they provide will be made under the same open source license that the open source project is provided under.

  • I acknowledge that all my contributions will be made under the project's license.
Go back to non-parameterized resource classes for DW servers
Arguably the old way made the generated code a little nicer to read, but
the aesthetics for the user were ugly with the type param, and
especially the wildcard type param in the handler signatures.

This effectively reverts acd9f59
.fromList(responses.value.collect({
case Response(statusCodeName, Some(_)) => statusCodeName
}))
.map(_.reverse.foldLeft[IfStmt](null)({

This comment has been minimized.

Copy link
@kelnos

kelnos Apr 10, 2019

Author Member

Before you scream weep in the corner be horribly sad at me for using null as a foldLeft initializer, let me explain. This actually does make sense and is weirdly elegant. The constructor signature for IfStmt is

public IfStmt(Expression condition, Statement thenStmt, Statement elseStmt)

So for the last item in the list, null has to be passed for elseStmt. In the old old old similar code, I mapped the response-with-entity list into a List[IfStmt] and passed null for elseStmt each time, and then did a side-effect-ful reduceLeft to just call prevStmt.setElse(curStmt) to mutate the existing statements. It worked, but I considered that ugly, as well as requiring an extra pass over the list.

This feels more elegant to me: reverse the list of stuff, pass null as the initializer, and then pass that as elseStmt in the lambda body. One iteration over the list, and no side-effect-ful mutation later.

This comment has been minimized.

Copy link
@blast-hardcheese

blast-hardcheese Apr 10, 2019

Collaborator

I definitely agree with you, especially considering the use of NonEmptyList. I wish that were able to be captured in the types, but all I've got is

def elif(elseClause: IfStmt, statusCodeName: String): IfStmt = ???
...map(_.reverse match { case NonEmptyList(last, rest) => rest.foldLeft(elif(null, last))(elif _)

which requires another definition. The only danger of what you have here is if NonEmptyList is relaxed, we leak null

This comment has been minimized.

Copy link
@kelnos

kelnos Apr 10, 2019

Author Member

Relaxed in the sense that they allow a NEL to be empty??

@kelnos kelnos requested a review from blast-hardcheese Apr 10, 2019

@kelnos kelnos merged commit 5b516f3 into twilio:master Apr 10, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@kelnos kelnos deleted the kelnos:dropwizard-response-class-unrefactor branch Apr 10, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.