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 Paths #771

Merged
merged 1 commit into from
Apr 30, 2017
Merged

Add Paths #771

merged 1 commit into from
Apr 30, 2017

Conversation

vkostyukov
Copy link
Collaborator

@vkostyukov vkostyukov commented Apr 26, 2017

I propose we restructure our "path matching" and "path extracting endpoints" around the DecodePath[A] type-class that has been available for quite a while already (as an extension point for matching/extracting arbitrary types).

Here is the changes:

  • There is now path[A] method that's intended to be a replacement for string, int, etc.
  • There is now explicit path("foo") method that makes it easier to convert arbitrary strings into matching endpoints (we could only use implicit conversion before).
  • There are some new laws around matching/extracting endpoints
  • By analogy with Bodies, there is now Paths that embeds all of the path-based endpoints.

I'd like to solicit some feedback on that given it's an API change. I understand path[Int] is way more verbose than int, but I really admire how explicit and extensible it is.

@codecov-io
Copy link

codecov-io commented Apr 26, 2017

Codecov Report

Merging #771 into master will increase coverage by 0.95%.
The diff coverage is 79.48%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #771      +/-   ##
==========================================
+ Coverage   81.51%   82.46%   +0.95%     
==========================================
  Files          37       38       +1     
  Lines         649      650       +1     
  Branches       22       22              
==========================================
+ Hits          529      536       +7     
+ Misses        120      114       -6
Impacted Files Coverage Δ
core/src/main/scala/io/finch/Endpoints.scala 85.41% <ø> (+9.83%) ⬆️
core/src/main/scala/io/finch/Endpoint.scala 76.11% <0%> (-1.75%) ⬇️
core/src/main/scala/io/finch/DecodePath.scala 100% <100%> (ø) ⬆️
core/src/main/scala/io/finch/endpoint/path.scala 85.71% <85.71%> (ø)

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 266d007...43e18f3. Read the comment docs.

@rpless
Copy link
Collaborator

rpless commented Apr 26, 2017

I like bullet points 2-4 and am all in favor of having those.
That said, I don't think we should remove string, int, and etc. I prefer them because they are short, readable, and I suspect a little better in situations when you don't need the extensible aspects of path. This will also be a fairly large breaking API change.

I see all the benefits of having path[Int], etc so I'd like to propose that we:

  • keep string, int, and friends defined in terms of path
  • migrate the documentation to use path and say that it is the preferred way of doing

If we go this route, I'm happy to run through the documentation and make that change.

@vkostyukov
Copy link
Collaborator Author

vkostyukov commented Apr 26, 2017

Thanks @rpless! I think that sounds reasonable. I'm happy to un-deprecate int, string, etc variants and see if the new path[A] API gets some level of adoption.

Just to add some more color on why I think it makes sense to get rid of int, string, entry-points. Some time ago, we treated Endpoints (Routers at that time) and RequestReaders differently and there was ? combinator allowing to compose them. At that time, something like int :: string ? param[Int] looked pretty reasonable I guess.

What really concerns me today is that int :: param[Int] may seem quite confusing given that it's really hard to tell (if you're new to Finch) what int means - is that a header, or maybe a body (you have to go and read the doc)? With path[Int] it becomes quite clear what's going on just looking at the code.

I agree it's slightly more verbose, but I personally think the clarity it brings in is a good enough reason to trade-off a couple of more characters in the source code.

@rpless
Copy link
Collaborator

rpless commented Apr 26, 2017

Yeah I understand the concerns about ambiguity, which is why I suggest documenting path[...] as the way to do things and leaving the existing endpoints (int, etc) as a more "advanced" feature.

I'd be curious to hear feedback from others and from people using it when this rolls out, but if the general consensus is that path is better then what we have now, then I think we should revisit deprecating the other endpoints.

@vkostyukov
Copy link
Collaborator Author

So this is pretty much ready to go alive. The only thing I decided to keep for now is removing named path extractors, eg string("foo"). They only affected toSring output. I'm wondering if we should try making this machinery more explicit, similar how scodec does it.

As a compromise, I can bring int("foo") syntax back and add withToString as a new API entry point.

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.

3 participants