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

Add QueryParamEncoder[Instant] and QueryParamDecoder[Instant] #2724

Merged
merged 18 commits into from Aug 11, 2019

Conversation

@kevinmeredith
Copy link
Contributor

@kevinmeredith kevinmeredith commented Jul 16, 2019

Addresses part of #2670.

Please take a look, @rossabaker and @LukaJCB.

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Jul 16, 2019

It seems slightly better to me if a single function produces both a QueryParamEncoder and QueryParamDecoder. Otherwise, a QueryParamEncoder[Instant] could be created with DateTimeFormatter.A, and then QueryParamDecoder[Instant] could be constructed with DateTimeFormatter.B.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

Would a function that returns both return a tuple? There's no QueryParamCodec, or is that what you're proposing?

Loading

def instantQueryParamDecoder(formatter: DateTimeFormatter): QueryParamDecoder[Instant] =
new QueryParamDecoder[Instant] {
override def decode(value: QueryParameterValue): ValidatedNel[ParseFailure, Instant] =
Either
Copy link
Member

@rossabaker rossabaker Jul 17, 2019

Choose a reason for hiding this comment

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

Validated has catchOnly and is a Bifunctor. I don't think you need the Either.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Aug 6, 2019

Choose a reason for hiding this comment

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

Done - 924c996

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Jul 17, 2019

Would a function that returns both return a tuple?

That seems reasonable to me, @rossabaker.

There's no QueryParamCodec, or is that what you're proposing?

Good question. I wasn't proposing that route. Looking at https://github.com/circe/circe/blob/v0.12.0-M4/modules/core/shared/src/main/scala/io/circe/Codec.scala#L7-L8, it shows that Codec is for convenience purposes only. What are your thoughts, @rossabaker?

Loading

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Jul 17, 2019

There is a good discussion of codec at circe/circe#133. I tend to agree with the desiderata and conclusion there.

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Aug 6, 2019

There is a good discussion of codec at circe/circe#133. I tend to agree with the desiderata and conclusion there.

Thanks for pointing me to that link, @rossabaker. Could you please review the recent changes that I've made?

Loading

Use types to improve documentation.
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Aug 8, 2019

I closed, and then opened because a flaky test appears to have been the problem: https://travis-ci.org/http4s/http4s/jobs/568317642.

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Aug 8, 2019

Hey @ChristopherDavenport @LukaJCB, please take a look cc @rossabaker. Thanks!

Loading

decodeA.decode(value)
}

private def instantQueryParamDecoder(formatter: DateTimeFormatter): QueryParamDecoder[Instant] =
Copy link
Member

@rossabaker rossabaker Aug 9, 2019

Choose a reason for hiding this comment

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

Why would this and instanctQueryParamDecoder not go on the companions of their respective typeclasses? The instantQueryParamCodec to join them makes sense, as does being explicit (because there's no default format).

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Aug 9, 2019

Choose a reason for hiding this comment

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

Good point, Ross. I'll address that.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Aug 9, 2019

Choose a reason for hiding this comment

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

I initially thought you suggested moving instantQueryParamDecoder and instantQueryParamEncoder to QueryParamDecoder and QueryParamEncoder respectively. However, I already took that approach.

In short, I don't entirely follow your requested changes. Could you please say more, @rossabaker?

Loading

Copy link
Member

@rossabaker rossabaker Aug 9, 2019

Choose a reason for hiding this comment

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

Aren't they in QueryParamCodec still? Maybe I need to check this out and not squint at a GitHub diff.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Aug 10, 2019

Choose a reason for hiding this comment

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

My mistake, Ross. You're right, instantQueryParamDecoder and instantQueryParamEncoder are defined in object QueryParamCodec. I'll move them to their respective companions. Sorry about that.

Loading

Copy link
Contributor Author

@kevinmeredith kevinmeredith Aug 10, 2019

Choose a reason for hiding this comment

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

Done in the commits added at #2724 (commits).

Loading

Loading
@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Aug 10, 2019

I'm figuring the build failed due to a flaky spec, @rossabaker?

[error] Failed: Total 65, Failed 1, Errors 0, Passed 61, Skipped 3, Pending 1
[error] Failed tests:
[error] org.http4s.client.blaze.Http1ClientStageSpec

Loading

@kevinmeredith
Copy link
Contributor Author

@kevinmeredith kevinmeredith commented Aug 10, 2019

Maybe 212e350 will fix the, I'm guessing, flaky test.

Loading

Copy link
Member

@rossabaker rossabaker left a comment

No, unfortunately those blaze-client tests have been flaky on CI and only CI for a long time. I'll restart it, but this is not related to this PR.

Loading

@aeons aeons merged commit 1f823bc into http4s:series/0.20 Aug 11, 2019
2 checks passed
Loading
@kevinmeredith kevinmeredith deleted the query-support-instant branch Aug 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants