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

Implement a matrix path variable parser #3844

Merged
merged 6 commits into from Nov 15, 2020

Conversation

rzeigler
Copy link
Contributor

Approximates what is described at https://www.w3.org/DesignIssues/MatrixURIs.html

@rzeigler rzeigler changed the base branch from main to series/0.21 Nov 11, 2020
@rossabaker rossabaker added the enhancement Feature requests and improvements label Nov 12, 2020
Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Thanks. Before I dive too deep into the implementation, a couple quick design thoughts.

}

"to many axes" in {
(Path("/board/square;x=42y=0;z=39") match {
Copy link
Member

Choose a reason for hiding this comment

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

Is this supposed to be x=42;y=0? And why is this invalid? I might have expected extra axes to be ignored.

Copy link
Contributor Author

@rzeigler rzeigler Nov 12, 2020

Choose a reason for hiding this comment

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

The similarity to query parameters probably means that they should be. I just need to make the toAssocList stack safe since its currently capped at the number of parameters that are expected to be extracted.

abstract class MatrixVar[F[_]: Traverse, A](
name: String,
domain: F[String],
range: String => Option[A]) {
Copy link
Member

Choose a reason for hiding this comment

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

In common use, are all parameters in the matrix of the same type?

/address;city=Indianapolis;zipCode=46268

Do I have to represent zipCode as a String? If we stripped range and returned Strings, could we nest extractors?

case Root / AddressVar(city, Zip(zipCode)) =>

Copy link
Contributor Author

@rzeigler rzeigler Nov 12, 2020

Choose a reason for hiding this comment

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

I'm not really sure there is a common use. The problem I was specifically looking for was a means to solve awkward urls like /board/x/0/y/1 which implies a hierarchical structure of resources where none exists. Beyond that the goal is something that more people than just me is an ergonomic API. range is basically a mini-extractor so I will make this change.

rzeigler added 2 commits Nov 12, 2020
The MatrixVar extractor is an unapplySeq for String now.
MatrixVar is more permissive allowing empty kv segments and an empty name.
@rzeigler
Copy link
Contributor Author

I've tried to address the comments and also make what is excepted more permissive.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

Neat!

@rossabaker rossabaker merged commit 11cdbd3 into http4s:series/0.21 Nov 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests and improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants