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

[#98] implementing query parameters building methods #99

Merged
merged 2 commits into from
Mar 20, 2018
Merged

[#98] implementing query parameters building methods #99

merged 2 commits into from
Mar 20, 2018

Conversation

vitaliihonta
Copy link
Contributor

No description provided.

@codecov
Copy link

codecov bot commented Mar 19, 2018

Codecov Report

Merging #99 into master will increase coverage by 0.46%.
The diff coverage is 78.26%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #99      +/-   ##
==========================================
+ Coverage   65.11%   65.58%   +0.46%     
==========================================
  Files          22       22              
  Lines         579      584       +5     
  Branches       10        8       -2     
==========================================
+ Hits          377      383       +6     
+ Misses        202      201       -1
Impacted Files Coverage Δ
core/shared/src/main/scala/hammock/package.scala 50% <66.66%> (+16.66%) ⬆️
core/shared/src/main/scala/hammock/Uri.scala 84.93% <82.35%> (+0.64%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6c05267...0bb3226. Read the comment docs.

Copy link
Owner

@pepegar pepegar left a comment

Choose a reason for hiding this comment

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

Looks very good, @vitaliihonta. I added a couple of minor comments only.

* @param value - the value
* @return updated [[Uri]]
**/
def param(key: String, value: String): Uri = copy(query = this.query + (key → value))
Copy link
Owner

Choose a reason for hiding this comment

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

Can you please use the non-unicode version of ?

* @param ps - parameters
* @return updated [[Uri]]
**/
def ?(ps: Seq[(String, String)]): Uri = params(ps: _*)
Copy link
Owner

Choose a reason for hiding this comment

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

Since Seq can be mutable, can you specify it to a more suitable data structure, such as List or Vector?

@@ -1,38 +1,42 @@
import scala.language.higherKinds
Copy link
Owner

Choose a reason for hiding this comment

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

We activate this flag in the build.sbt already, IIRC. Can you delete this?

@vitaliihonta
Copy link
Contributor Author

vitaliihonta commented Mar 19, 2018

@pepegar Ok, I'm fixing.
One more question. I have'nt found a function deriving Codec having Decoder and Encoder as implicits in scope. Also I have'nt found Codec[String]
I'm using such code in our project:

  implicit object StringEncoder extends Encoder[String] {
    def encode(a: String): Entity = Entity.StringEntity(a)
  }
  implicit object StringDecoder extends Decoder[String] {
    override def decode(a: Entity): Either[CodecException, String] = a match {
      case Entity.StringEntity(body, _)        Right(body)
      case Entity.ByteArrayEntity(bodyArr, _)  Right(new String(bodyArr, StandardCharsets.UTF_8))
    }
  }
  implicit def deriveCodec[A](implicit D: Decoder[A], E: Encoder[A]): Codec[A] = new Codec[A] {
    override def decode(a: Entity): Either[CodecException, A] = D.decode(a)
    override def encode(a: A): Entity = E.encode(a)
  }

Should I implement such things within this PR?
P.S. Without unicode arrows =)

@pepegar
Copy link
Owner

pepegar commented Mar 19, 2018

Hey @vitaliihonta!

Thank you very much for this PR, looks fine to merge now.

I will be merging #96 afterwards, so I'll rebase on this before.

As for the other changes, you're right that we don't have an implicit method that brings an Codec[A] given a Decoder[A] and a Encoder[A]. I think it's better to implement them in another pull request. We could have such method in the companion object of Codec, what do you think?

If you're happy with the changes here, I'll merge tomorrow morning :)

@vitaliihonta
Copy link
Contributor Author

Hey @pepegar
I understood. So I’ll implement these features after merge in another brunch

@pepegar
Copy link
Owner

pepegar commented Mar 20, 2018

Merging, thanks @vitaliihonta !

@pepegar pepegar merged commit 50548dd into pepegar:master Mar 20, 2018
@pepegar pepegar mentioned this pull request Mar 20, 2018
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

2 participants