Split files and multipart attributes and enable type parameters for attributes #879
Conversation
Codecov Report
@@ Coverage Diff @@
## master #879 +/- ##
==========================================
- Coverage 80.13% 79.69% -0.44%
==========================================
Files 52 52
Lines 765 788 +23
Branches 31 31
==========================================
+ Hits 613 628 +15
- Misses 152 160 +8
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hey @imliar! This looks good to me - just a few nits. Thanks for working on this!
| import io.finch._ | ||
| import io.finch.internal.Rs | ||
|
|
||
| private abstract class Attribute[F[_], A](val name: String, val d: DecodeEntity[A], val tag: ClassTag[A]) |
There was a problem hiding this comment.
I'd vote we keep both Attribute and FileUpload under the same file, multipart.scala. We do that for params.
| @@ -15,6 +15,7 @@ package object finch extends Endpoints | |||
| sealed abstract class RequestItem(val kind: String, val nameOption:Option[String] = None) { | |||
| val description = kind + nameOption.fold("")(" '" + _ + "'") | |||
| } | |||
| final case class AttributeItem(name: String) extends RequestItem("attribute", Some(name)) | |||
There was a problem hiding this comment.
Why do you think we have to introduce a new request item for attributes? We've been using ParamItem until now and I actually think it's perfectly reasonable.
There was a problem hiding this comment.
Well, my idea behind it is that they are different entities, so different items. But if you have a strong opinion on that, I could remove it
There was a problem hiding this comment.
My personal preference is to keep using ParamItem just b/c
- We've been using it before and
- I really want to see us removing
RequestItemthing all-together at some point (and possibly replacing with something else)
|
|
||
| protected def missing(name: String): Rerunnable[Output[F[A]]] | ||
| protected def present(value: NonEmptyList[A]): Rerunnable[Output[F[A]]] | ||
| protected def notparsed(errors: NonEmptyList[Throwable]): Rerunnable[Output[F[A]]] |
| } | ||
| } | ||
|
|
||
| private trait SingleItem[F[_], A] { _: Attribute[F, A] => |
There was a problem hiding this comment.
Let's move both SingleItem and MultipleItems under the Attribute object. Also, what do you think of renaming them to SingleError and MultipleErrors correspondingly?
project/build.properties
Outdated
| @@ -1 +1 @@ | |||
| sbt.version=1.0.3 | |||
| sbt.version=1.0.4 | |||
There was a problem hiding this comment.
Also you should rebase - I think we already upgraded sbt.
sergeykolbasov
force-pushed
the
attributes-decoder
branch
from
d012344 to
610bfc6
Compare
|
@vkostyukov done |
|
Thanks again, @imliar! This looks good to me. |
Related to #786
API should stay the same, just a slight refactoring