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

Optimise dsl #3876

Merged
merged 6 commits into from Nov 18, 2020
Merged

Optimise dsl #3876

merged 6 commits into from Nov 18, 2020

Conversation

ashwinbhaskar
Copy link
Collaborator

No description provided.

@ashwinbhaskar ashwinbhaskar changed the title WIP Optimise dsl Optimise dsl Nov 17, 2020
@ashwinbhaskar ashwinbhaskar requested review from rossabaker and hamnis Nov 17, 2020
Copy link
Contributor

@hamnis hamnis left a comment

Choose a reason for hiding this comment

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

Thanks for picking this up.
The Segment constructor should probably not be accessible outside of Path.

The path string interpelator or Path.fromString should be preferred when constructing paths.

"//foo/bar" in {
Path("//foo/bar") must_== Path("", "foo", "bar")
"~ extractor on Path without Root" in {
(Path(Vector(Segment("foo.json"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use path"foo.json" or Path.fromString

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@hamnis I am so sorry. I thought I had changed all to Path.fromString. Will change it and push

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

"~ extractor on Path" in {
(Path("/foo.json") match {
"~ extractor on Path with Root" in {
(Path(Vector(Segment("foo.json")), absolute = true) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

you can use path"/foo.json" or Path.fromString

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}

"/: should not crash without trailing slash" in {
// Bug reported on Gitter
Path("/cameras/1NJDOI") match {
Path(Vector(Segment("/cameras/1NJDOI"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

this cant be right.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

case Root / "user" / IntVar(userId) => userId == 123
case _ => false
}) must beTrue
}

"Int extractor, invalid int" in {
(Path("/user/invalid") match {
(Path(Vector(Segment("/user/invalid"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

This cant be right?
Path.fromString should be used here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

case Root / "user" / UUIDVar(userId @ _) => true
case _ => false
}) must beFalse
}
"a bad UUID" in {
(Path("/user/13251d88-7a73-4fcf-b935") match {
(Path(Vector(Segment("/user/13251d88-7a73-4fcf-b935"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Path.fromString

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@@ -302,55 +272,47 @@ class PathSpec extends Http4sSpec {

"invalid" >> {
"empty with semi" in {
(Path("/board/square;") match {
(Path(Vector(Segment("/board/square;"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Path.fromString

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"empty without semi" in {
(Path("/board/square") match {
(Path(Vector(Segment("/board/square"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

case Root / "board" / BoardExtractor(x @ _, y @ _) => true
case _ => false
}) must beFalse
}

"empty with mismatched name" in {
(Path("/board/other") match {
(Path(Vector(Segment("/board/other"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

same

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

case Root / "user" / UUIDVar(userId @ _) => true
case _ => false
}) must beFalse
}
"a word" in {
(Path("/user/invalid") match {
(Path(Vector(Segment("/user/invalid"))) match {
Copy link
Contributor

Choose a reason for hiding this comment

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

Path.fromString

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ashwinbhaskar
Copy link
Collaborator Author

@hamnis @rossabaker I had to make changes in PrometheusClientMetricsSpec and DropwizardClientMetricsSpec as the tests were failing because the the uris were missing a leading / and were being matched against Root / ....

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.

This is looking nice to me, but @hamnis is much more of an expert in this area of the code now, so I'll defer to him.

@@ -361,7 +361,7 @@ object Uri {
val (start, end) = segments.splitAt(idx)
Path(start, absolute = absolute) -> Path(end, true, endsWithSlash = endsWithSlash)
}
private def copy(
def copy(
Copy link
Member

Choose a reason for hiding this comment

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

Are there any invariants we might be breaking because of making this public? Certain invalid combinations we don't want to allow? I think all states are legal.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rossabaker really sorry, I did not understand why/how does variance come into the picture here?

Copy link
Member

Choose a reason for hiding this comment

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

Invariants (e.g., constraints), not invariance (e.g., subtypes and supertypes). Invariants are properties of a class that can't be violated. Silly example:

class Date(month: Int, day: Int, year: Int)

Invariants of Date might be that month >= 1 and month <= 12 and if month == 4 then date <= 30. When we expose a copy constructor, we want to e sure that all parameters to it are legal, and all combinations are legal. And whenever a constructor is private, it's often the case that copy should be private.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rossabaker ah okay, got it. Makes sense. I used the copy method purely for convenience. Should I revert it?

Copy link
Member

Choose a reason for hiding this comment

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

Let's see what @hamnis says. If it's dangerous, we can revert and add appropriate with* methods. If it's safe, we can keep the convenience. Either way, we should have a clear path forward.

object / {
def unapply(path: Path): Option[(Path, String)] =
if (path.endsWithSlash)
Some(path.copy(endsWithSlash = false) -> "")
Copy link
Contributor

Choose a reason for hiding this comment

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

path.dropEndsWithSlash

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

}
implicit val arbitraryPath: Gen[Path] =
arbitrary[List[String]]
.map(_.foldLeft(Path.Root)((acc, e) => acc.copy(segments = acc.segments :+ Segment(e))))
Copy link
Contributor

Choose a reason for hiding this comment

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

acc.addSegment(e) or acc / e

that should eliminate both uses of copy

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

done

@ashwinbhaskar ashwinbhaskar requested a review from hamnis Nov 18, 2020
hamnis
hamnis approved these changes Nov 18, 2020
Copy link
Contributor

@hamnis hamnis left a comment

Choose a reason for hiding this comment

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

LGTM

@rossabaker rossabaker merged commit 4b6e13a into http4s:main Nov 18, 2020
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

3 participants